Opened 3 years ago

Closed 3 years ago

#11788 closed Bug (fixed)

It is not possible to change language back to undefined in code snippet dialog.

Reported by: pjasiun Owned by: pjasiun
Priority: Normal Milestone: CKEditor 4.4.1
Component: General Version: 4.4.0
Keywords: Cc:

Description

  1. Open code snippet sample.
  2. Open edit dialog for code snippet with undefined language.
  3. Change language to PHP (or whatever).

You are not able to change it back to undefined.

Change History (15)

comment:1 Changed 3 years ago by pjasiun

  • Milestone set to CKEditor 4.4.1

comment:2 Changed 3 years ago by j.swiderski

  • Status changed from new to confirmed

comment:3 Changed 3 years ago by pjasiun

  • Owner set to pjasiun
  • Status changed from confirmed to assigned

comment:4 Changed 3 years ago by pjasiun

  • Status changed from assigned to review

Changes in t/11788 and corresponding test branch.

comment:5 Changed 3 years ago by Reinmar

  • Status changed from review to review_failed

TC:

  1. Open code snippet sample.
  2. Dblclick first snippet.
  3. Change language to <not set> and close dialog.
  4. Switch to source - the snippet still has language-javascript class.

So there's an additional bug somewhere. Remember about writing test for it.

One more thing worries me. I wanted to write:

Add the empty option to the code snippet in codeSnippet_languages doc. Remember that there's no editor instance...

And then I realised that it's indeed impossible to reconfigure languages list using the translations. Thoughts? Maybe the name of language '' should always be taken from lang.common.notSet? But then - the value of codeSnippet_languages[''] should be set to any truly value? Don't we overcomplicate this?

Last edited 3 years ago by Reinmar (previous) (diff)

comment:6 Changed 3 years ago by Reinmar

I consulted others and we made a decision that we'll always include the empty option - it does not have to be configurable now. We'll wait for feedback.

comment:7 Changed 3 years ago by pjasiun

  • Status changed from review_failed to review

Bug fixed, test added, changes in t/11788.

And I also think that we should keep it simple, if someone need to reconfigure languages list using the translations he can define a new highlighter.

comment:8 Changed 3 years ago by Reinmar

  • Status changed from review to review_failed

So why didn't you make it simpler? ;) In proposed branch:t/11788 you still have the same problem which I described in comment:5. Empty option is still inside the languages list and if you configure languages through config.codeSnippet_languages you still have problem with the "not set" translation.

The empty option should be removed from config property and should be added regardless of the config property value.

Last edited 3 years ago by Reinmar (previous) (diff)

comment:9 Changed 3 years ago by pjasiun

  • Status changed from review_failed to review

We cannot remove empty value from languages and move it to the setHighlighter because then it would not be possible to create list of languages with no empty option, even using custom highlighter.`

Instead, I put an empty string into languages list and replace this sting with lang.common.notSet in setHighlighter.

Changes in t/11788.

comment:10 Changed 3 years ago by Reinmar

  • Status changed from review to review_failed

In comment:6 I wrote that we decided to KISS for now and always include empty option. You agreed, so why did you push the previous approach again?

Beside wrong solution branch:t/11788 has following issues:

  • It's overcomplicated as mentioned in comment:5.
  • The plugin's code changes config object by setting lang[''] = notSet. To fix that you would have to complicate the implementation even more. That's why we decided to not go this way.

comment:11 Changed 3 years ago by pjasiun

  • Status changed from review_failed to review

Ok, I changed the solution, and pushed to the t/11788b and corresponding test branch.

comment:12 Changed 3 years ago by Reinmar

  • Status changed from review to review_passed

I rebased branches and pushed one small commit to dev. Mind the details, PJ.

comment:13 Changed 3 years ago by Reinmar

I've got one small doubt still. When language is not set codesnippet will not run highlighter. However, when loading data with a snippet without chosen language, some hilghlighters, for example highlight.js, will try to guess the language and colorize the snippet. I checked highlight.js and it does it poorly.

So should we run highlighter even though language was not set or can we ignore this case?

There's also one more thing - sometimes a content author may want to leave language unspecified for a reason - e.g. because it's an ASCII art (like our cow). Highlight.js ran on it will break it... So we somehow miss the "plain text" option. Or the answer is above - we should not run highlighter when language is not set. And on the target page it's developers choice what to do.

comment:14 Changed 3 years ago by pjasiun

I think that we definitely should not run highlighter if no language is defined. What it do is a mess and probably many people disable such behavior on they websites (I did it last time I was working with highlighter).

comment:15 Changed 3 years ago by pjasiun

  • Resolution set to fixed
  • Status changed from review_passed to closed
Note: See TracTickets for help on using tickets.
© 2003 – 2016 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy