Ticket #5915 (closed New Feature: fixed)

Opened 4 years ago

Last modified 3 years ago

ImageDlgHideLink not ported from V2

Reported by: wwalc Owned by: tobiasz.cudnik
Priority: Normal Milestone: CKEditor 3.5
Component: UI : Dialogs Version: 3.0
Keywords: Cc:

Description

Either I'm missing something or we forgot to port those settings:

  • ImageDlgHideLink
  • ImageDlgHideAdvanced
  • FlashDlgHideAdvanced

We have ported only:

  • linkShowAdvancedTab
  • linkShowTargetTab

Attachments

#5915.patch (2.6 KB) - added by mani 4 years ago.
Patch
5915_2.patch (1.4 KB) - added by mani 4 years ago.
5915_3.patch (1.5 KB) - added by mani 4 years ago.
5915_4.patch (1.4 KB) - added by mani 3 years ago.
5915_5.patch (1.4 KB) - added by mani 3 years ago.
5915_6.patch (1.5 KB) - added by mani 3 years ago.
5915_7.patch (2.0 KB) - added by mani 3 years ago.
5915_8.patch (2.1 KB) - added by mani 3 years ago.
5915_9.patch (2.2 KB) - added by Saare 3 years ago.
5915_10.patch (2.1 KB) - added by tobiasz.cudnik 3 years ago.
5915_11.patch (2.5 KB) - added by tobiasz.cudnik 3 years ago.
5915_12.patch (2.1 KB) - added by tobiasz.cudnik 3 years ago.

Change History

comment:1 Changed 4 years ago by alfonsoml

I proposed to remove the link options in #5365 as they aren't included in the docs, but Fred said to leave it as is.

This doc explains how to get the same result using the API so it can be used for any other dialog or tab.

comment:2 Changed 4 years ago by fredck

  • Keywords Discussion added; Confirmed removed

It's a very bad solution to have such static settings for these things. Right now, as pointed by Alfonso, the API is the right way for it, as it's generic, and can be applied to all kinds of dialogs.

What we could think about instead, is having some kind of "shortcut" for some common operations, just to make people's life easier.

For example, we could add checks on the dialog system, which looks for a configuration composed by "<dialog name> + Show + <tab name> + Tab", and automatically show/hide it based on that.

Something similar could be done to show/hide dialog elements, as well as setting their default values.

comment:3 Changed 4 years ago by alfonsoml

Yes, while writting the reply this morning I saw that it can be done "easily" with one configuration entry to specify the tabs to remove from each dialog (or elements in a dialog).

I don't see the need to allow to specify if it must be shown or hidden, by default everything is there, so the config should be to specify which elements must be removed removeDialogContents = "Image:Advanced;Link:Advanced"

If we implement #5105 the option to remove elements can be just as easy as specify its id in the same way: removeDialogContents = "Image:Advanced;Link:Advanced;Image:Border"

comment:4 Changed 4 years ago by fredck

  • Keywords Confirmed added; Discussion removed

Yes, that's the kind of generic approach we could have for it, and it's a good solution. So, let's go ahead with it.

comment:5 Changed 4 years ago by fredck

  • Milestone changed from CKEditor 3.4 to CKEditor 3.5

comment:6 Changed 4 years ago by fredck

  • Milestone changed from CKEditor 3.4.1 to CKEditor 3.5

comment:7 Changed 4 years ago by mani

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

Changed 4 years ago by mani

Patch

comment:8 Changed 4 years ago by mani

  • Status changed from assigned to review

comment:9 Changed 4 years ago by tobiasz.cudnik

  • Status changed from review to review_failed

Patch is OK, but coding style is wrong - please use tabs for indentation. Theres also unneeded change on L310 of _source/core/config.js file.

comment:10 Changed 4 years ago by fredck

Additionally, the removeDialogContents doesn't need to be initialized. It can be a "documentation only" setting, which is btw restricted to the dialog plugin, so it should not go into the core config file.

And yes, some reading about coding style and patterns would be good ;)

Changed 4 years ago by mani

comment:11 Changed 4 years ago by mani

  • Status changed from review_failed to review

comment:12 Changed 4 years ago by Saare

  • Status changed from review to review_failed
  • There are still coding style mistakes. Please read the coding style and patterns pages.
  • As Fred commented before, the setting doesn't need to be initialized. Instead, please add a descriptive comment about it. Example for such a comment can be found here.

Changed 4 years ago by mani

comment:13 Changed 3 years ago by mani

  • Status changed from review_failed to review

comment:14 Changed 3 years ago by Saare

  • Status changed from review to review_failed

Small things:

  • Why i <= removeContentDefinition.length - 1 and not simply i < removeContentDefinition.length?
  • For the same line, please take a look at here.
  • Please add the '@since 3.5' tag to the comment.
  • Please update your local copy before making any new patches, and make sure you're working on the correct branch.

comment:15 Changed 3 years ago by garry.yao

In the API doc config doc sample comma is instead used as separator, also, as this configuration would probably break a dialog (because of a major tab missing) we need a disclaimer in the documentation.

Changed 3 years ago by mani

comment:16 Changed 3 years ago by mani

  • Status changed from review_failed to review

Changed 3 years ago by mani

comment:17 Changed 3 years ago by Saare

  • Status changed from review to review_failed

Let's cache the split string, so we won't have to split it every time a dialog is opened. Also, please add the disclaimer described by Garry.

Changed 3 years ago by mani

comment:18 Changed 3 years ago by mani

  • Status changed from review_failed to review

comment:19 Changed 3 years ago by tobiasz.cudnik

  • Status changed from review to review_failed

As i see 5915_6.patch still doesn't cache the result of the split method as suggested by Saare. Just the variable for a config property has been added.

Changed 3 years ago by mani

comment:20 Changed 3 years ago by mani

  • Status changed from review_failed to review

comment:21 Changed 3 years ago by Saare

  • Status changed from review to review_failed
  • Instead of removeDialog, let's call it removeDialogContents.
  • Why not to have an object to store the tabs to remove, instead of two-dimensional array? { dialogName: [ 'dialogTab', 'otherTab' ] } } for example.

Changed 3 years ago by mani

comment:22 Changed 3 years ago by mani

  • Status changed from review_failed to review

comment:23 Changed 3 years ago by Saare

  • Status changed from review to review_failed

You misunderstood my idea, and the patch seems to be made for trunk (should be made on the 3.5 branch). I will post a new patch in a few minutes.

comment:24 follow-up: ↓ 26 Changed 3 years ago by tobiasz.cudnik

All names should be non case sensitive, as it creates problems when defining a config.

Additionally, used data structure is completely different than proposed one. It's about L198:

tabsToRemove.push( { dialogName: dialogContent[ 0 ], tabName: dialogContent[ 1 ] }); 

Which result is:

[
  {
    dialogName: 'Link',
    tabName: 'Advanced'
  },
  {
    dialogName: 'Link',
    tabName: 'Link'
  },
  { ... },
  ...
]

comment:25 Changed 3 years ago by tobiasz.cudnik

  • Type changed from Bug to New Feature

Changed 3 years ago by Saare

comment:26 in reply to: ↑ 24 Changed 3 years ago by Saare

Replying to tobiasz.cudnik:

All names should be non case sensitive, as it creates problems when defining a config.

Let's bring #5138 into priority, and make this configuration case insensitive later.

comment:27 Changed 3 years ago by mani

  • Status changed from review_failed to review

comment:28 Changed 3 years ago by garry.yao

  • Status changed from review to review_failed

The latest patch is over complicated, even stranger seeing that for...in is used on an array, KISS pls.

comment:29 Changed 3 years ago by tobiasz.cudnik

  • Owner changed from mani to tobiasz.cudnik
  • Status changed from review_failed to assigned

Changed 3 years ago by tobiasz.cudnik

comment:30 Changed 3 years ago by tobiasz.cudnik

  • Status changed from assigned to review

comment:31 Changed 3 years ago by garry.yao

  • Status changed from review to review_failed

As we already said:

Note that the tab names and dialog names are case sensitive!

Then we have to leave the field name case untouched, e.g. broken example of the config 'image:Link'.

Changed 3 years ago by tobiasz.cudnik

comment:32 Changed 3 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

11th patch brings case insensivity with some simple changes, as present dialogs and their's contents names are strongly different.

comment:33 Changed 3 years ago by alfonsoml

I agree with Saare that the case sensitivity should be handled in #5138 instead of making this configuration a special situation.

comment:34 Changed 3 years ago by garry.yao

  • Status changed from review to review_failed

Please, leave the case sensitivity now and having other ticket deal with this issue.

Changed 3 years ago by tobiasz.cudnik

comment:35 Changed 3 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

comment:36 Changed 3 years ago by garry.yao

  • Status changed from review to review_passed

comment:37 Changed 3 years ago by tobiasz.cudnik

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

Fixed with [6145].

comment:38 Changed 3 years ago by fredck

  • Status changed from closed to reopened
  • Resolution fixed deleted

I see that the setting has been named "removeDialogContents", which made me think that I can remove anything from the dialog contents. But it looks like we can remove tabs only.

Please changed it's name to "removeDialogTabs", to make it clear.

comment:39 Changed 3 years ago by fredck

  • Component changed from General to UI : Dialogs

comment:40 Changed 3 years ago by tobiasz.cudnik

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

Post fixed with [6146].

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