ImageDlgHideLink not ported from V2

[Wiktor Walc]

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

• ImageDlgHideLink
• ImageDlgHideAdvanced
• FlashDlgHideAdvanced

We have ported only:

• linkShowAdvancedTab
• linkShowTargetTab

[Alfonso Martínez de Lizarrondo]

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.

[Frederico Caldeira Knabben]

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.

[Alfonso Martínez de Lizarrondo]

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"

[Frederico Caldeira Knabben]

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.

[Martin]

Patch

[Tobiasz Cudnik]

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.

[Frederico Caldeira Knabben]

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 ;)

[Sa'ar Zac Elias]

• 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.

[Sa'ar Zac Elias]

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.

[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.

[Sa'ar Zac Elias]

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.

[Tobiasz Cudnik]

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.

[Sa'ar Zac Elias]

• 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.

[Sa'ar Zac Elias]

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.

[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'
  },
  { ... },
  ...
]

[Sa'ar Zac Elias]

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.

[Garry Yao]

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

[Garry Yao]

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'.

[Tobiasz Cudnik]

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

[Alfonso Martínez de Lizarrondo]

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

[Garry Yao]

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

[Tobiasz Cudnik]

Fixed with [6145].

[Frederico Caldeira Knabben]

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.

[Tobiasz Cudnik]

Post fixed with [6146].