TBH creating a widget without adding it to a layout does not make much sense to me -- in the past I saw those situations as "floating" widgets usually anchored at the top-left corner of the parent widget.
The best way here is to just not create m_iconButton at all, like I suggested in D14378: Remove custom icon selection for trash too.

In D14360#298208, @shubham wrote:

pino: I don't why m_iconButton isn't needed(because it is being used by other entries also ), am I right?

I don't understandwhat you mean, can you please rephrase it?

The point is: if the icon must not be edited, then even creating the KiconButton for it is not useful, because it's an unused widget. Even more, Creating it and making it hidden still uses the resources needed to create it, and load the icon for it. So... just do not create it, taking care of handling in the dialog the case when m_iconButton is null.

pino, I understood your idea of having a helper class function, but the condition for editing the icon will remain the same ( based on its scheme), what can be the other condition?(maybe based on protocol?)

In D14360#299439, @shubham wrote:

pino, I understood your idea of having a helper class function, but the condition for editing the icon will remain the same ( based on its scheme), what can be the other condition?

For example if in the future you exclude another scheme from icon editing. Generally speaking, there is a logic here (url.scheme() != QLatin1String("trash")), and usually duplicating it even across the very same source file is not a good idea.

indirectly you mean to transfer all the logic to helper function

In D14360#299441, @shubham wrote:

indirectly you mean to transfer all the logic to helper function

No, I suggested something like:

bool KFilePlaceEditDialog::canEditIcon() const
{
  return m_iconButton;
}

using it in KFilePlaceEditDialog::getInformation().

pino: suppose this function is created(canEditIcon()),then this function must also be called when we are creating the m_iconButton at line 123 so as to generalize it's creation ( as you had said to take into account other entries also if they are also to be excluded).

In D14360#299443, @shubham wrote:

pino: suppose this function is created(canEditIcon()),then this function must also be called when we are creating the m_iconButton at line 123 so as to generalize it's creation ( as you had said to take into account other entries also if they are also to be excluded).

The function I suggested shows to the external users of KFilePlaceEditDialog (which is only the static KFilePlaceEditDialog::getInformation()) whether the icon could be edited or not.

ping?

Almost there! Please implement cfeck's suggestion for additional documentation for this function in the header file, in particular describing why come icons might not be editable.

In D14360#301252, @ngraham wrote:

Almost there! Please implement cfeck's suggestion for additional documentation for this function in the header file, in particular describing why come icons might not be editable.

I could't find the best way to describe it , any suggestions

Space is cheap. :) Verbosity is no problem as long as it communicates important information. How about this:

* @returns whether the item's icon is editable, as some icons are not (e.g. the Trash has the capability to display two icons, representing full and empty states, and it's far simpler to make these icons non-editable than is it to provide an interface to edit them both)

I still would like to understand why it needs to be public API, instead of just an @internal method. If I understand it correctly, the API isn't needed by any application, but only by the dialog itself. Also, if we decide to implement choosing both icons, or have a new icon format that supports multiple states, we don't need this API.

@cfeck agree with you(it's just a helper)
@ngraham what you reckon?

Yep, I agree.

In D14360#301270, @cfeck wrote:

I still would like to understand why it needs to be public API, instead of just an @internal method.

This is not a public API, since KFilePlaceEditDialog is a private class which is not even exported.

If it's not public, it doesn't need a @since tag.

Why isn't the header called *_p.h ...?

In D14360#301497, @cfeck wrote:

If it's not public, it doesn't need a @since tag.

No idea, it was not me to suggest that.

Why isn't the header called *_p.h ...?

No idea either.

ping?

@anthonyfieroni righty said , strictly it should return T/F

@dfaure I don't have commit access

I just noticed that this header says "const QString &icon() const;" i.e. we return a reference to a string. Assuming this API is really not public, it probably needs to be changed to a non-reference, correct?

In D14360#302141, @cfeck wrote:

Well, I had to look up your 'previous contributions' page to find your full name and mail address, and I would say given your rate of submits it makes sense to apply for a developer account so that you can commit yourself. This means less work for us.

Going to commit this one in a minute :)

@cfeck Thank you for committing it on my behalf. BTW can I give your name as supporter for KDE developer account application?

In D14360#302306, @cfeck wrote:

I just noticed that this header says "const QString &icon() const;" i.e. we return a reference to a string. Assuming this API is really not public, it probably needs to be changed to a non-reference, correct?

Yeah even @ngraham had said that it is actually a private header but not named so.
will probably rename it to a private header in another patch.

In D14360#302335, @shubham wrote:

can I give your name as supporter for KDE developer account application?

Sure (even if I hate the mail I will receive from sysadmins ;)

In D14360#302343, @cfeck wrote:

In D14360#302335, @shubham wrote:

can I give your name as supporter for KDE developer account application?

Sure (even if I hate the mail I will receive from sysadmins ;)

hahaha : )

I have now renamed the header to _p.h to avoid further confusion.