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: Piotr Jasiun Owned by: Piotr Jasiun
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 Piotr Jasiun

Milestone: CKEditor 4.4.1

comment:2 Changed 3 years ago by Jakub Ś

Status: newconfirmed

comment:3 Changed 3 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedassigned

comment:4 Changed 3 years ago by Piotr Jasiun

Status: assignedreview

Changes in t/11788 and corresponding test branch.

comment:5 Changed 3 years ago by Piotrek Koszuliński

Status: reviewreview_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 Piotrek Koszuliński (previous) (diff)

comment:6 Changed 3 years ago by Piotrek Koszuliński

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 Piotr Jasiun

Status: review_failedreview

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 Piotrek Koszuliński

Status: reviewreview_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 Piotrek Koszuliński (previous) (diff)

comment:9 Changed 3 years ago by Piotr Jasiun

Status: review_failedreview

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 Piotrek Koszuliński

Status: reviewreview_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 Piotr Jasiun

Status: review_failedreview

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

comment:12 Changed 3 years ago by Piotrek Koszuliński

Status: reviewreview_passed

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

comment:13 Changed 3 years ago by Piotrek Koszuliński

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 Piotr Jasiun

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 Piotr Jasiun

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