D14360: Remove custom icon selection for trash

[Shubham]

shubham created this revision.
shubham added reviewers: ngraham, broulik.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
shubham requested review of this revision.

REVISION SUMMARY
  BUG: 391200

TEST PLAN
  1. Right click trash on places panel
  2. Edit
  
  Result: No custom icon menu

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

AFFECTED FILES
  src/filewidgets/kfileplaceeditdialog.cpp

To: shubham, ngraham, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Kai Uwe Broulik]

broulik added a comment.


  Thanks a lot for your patch!
  
  I think it is cleaner if the button is setup but then hidden:
  
m_iconButton->setVisible(url.toString() != QLatin1String("trash:/"));
  
  Curiously, Dolphin overwrites the icon whenever `url.protocol() == "trash` 
whereas `KFilePlacesModel` only does it for `url.toString() == "trash:/"`. 
However you cannot actually add files inside Trash to Places in Dolphin (and 
why would you, you can from file dialog, though, but that is a bug) anyway.
  So perhaps can be simplified even to:
  
m_iconButton->setVisible(url.scheme() != QLatin1String("trash"));

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham added a comment.


  broulik: you mean the visibility of button will depend on the type of 
KFilePlacesItem(if its trash , its hidden, else not)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Kai Uwe Broulik]

broulik added a comment.


  If the URL of the places item is of scheme `trash` (Scheme is the part before 
the colon, e.g. `file`, `http`, …), then it should hide it, yes.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham updated this revision to Diff 38405.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14360?vs=38402&id=38405

REVISION DETAIL
  https://phabricator.kde.org/D14360

AFFECTED FILES
  src/filewidgets/kfileplaceeditdialog.cpp

To: shubham, ngraham, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  This works from Gwenview (for example), but does not work from Dolphin, 
because sadly Dolphin has its own duplicate of this code. :( Could you submit 
another patch for Dolphin too? It's the same change, to a file by the same 
name. :(
  
  We really really really need to eliminate this duplication...

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Nathaniel Graham]

ngraham added a reviewer: Dolphin.
ngraham requested changes to this revision.
ngraham added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kfileplaceeditdialog.cpp:123
>  m_iconButton = new KIconButton(this);
> +m_iconButton->setVisible(url.scheme() != QLatin1String("trash"));
>  layout->addRow(i18n("Choose an &icon:"), m_iconButton);
>  m_iconButton->setObjectName(QStringLiteral("icon button"));

Let's also avoid adding the button to the layout in this case, just like you're 
doing in D14378 .

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham updated this revision to Diff 38452.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14360?vs=38405&id=38452

REVISION DETAIL
  https://phabricator.kde.org/D14360

AFFECTED FILES
  src/filewidgets/kfileplaceeditdialog.cpp

To: shubham, ngraham, broulik, #dolphin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a reviewer: Frameworks.
ngraham added a comment.
This revision now requires changes to proceed.


  It's important to test your changes before submitting. :) This now causes 
Gwenview to segfault when you try to edit its Trash entry. The reason is 
because now that `m_iconButton` isn't added to the layout, the line beginning 
with `layout->labelForField()` crashes when it tries to run, since 
`m_iconButton isn't in a layout. That needs to be moved into the conditional as 
well.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham updated this revision to Diff 38468.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14360?vs=38452&id=38468

REVISION DETAIL
  https://phabricator.kde.org/D14360

AFFECTED FILES
  src/filewidgets/kfileplaceeditdialog.cpp

To: shubham, ngraham, broulik, #dolphin, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

pino added a comment.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

pino added a comment.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham added a comment.


  pino: I mean the very same m_iconButton is common to all other places entries 
like root,home etc, we are just creating it depending on whether it is trash or 
not

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham added a reviewer: dfaure.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, dfaure
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham updated this revision to Diff 38524.
shubham added a comment.


  Creating m_iconButton only for entries other than TRASH

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14360?vs=38468&id=38524

REVISION DETAIL
  https://phabricator.kde.org/D14360

AFFECTED FILES
  src/filewidgets/kfileplaceeditdialog.cpp

To: shubham, ngraham, broulik, #dolphin, #frameworks, dfaure
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

pino requested changes to this revision.
pino added a comment.
This revision now requires changes to proceed.


  It is a good start, but not enough:
  
  - `m_iconButton` must be set to null in the constructor (ideally at the 
beginning, in the initializer list), otherwise it will be uninitialized memory
  - `KFilePlaceEditDialog::icon()` will crash now, so its usage of 
`m_iconButton` must be guarded; either you return the icon passed to the 
constructor (which thus you need to save), or adjust callers to ask whether the 
icon could be changed, and only in that case get the icon

INLINE COMMENTS

> kfileplaceeditdialog.cpp:123
> +
> +// Draw the icon button for the entry only if it's *NOT* TRASH
> +if (url.scheme() != QLatin1String("trash")) {

No need to SCREAM in comments :)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, dfaure, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham updated this revision to Diff 38633.
shubham added a comment.


  Check before getting the icon, whether it's editable or not

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14360?vs=38524&id=38633

REVISION DETAIL
  https://phabricator.kde.org/D14360

AFFECTED FILES
  src/filewidgets/kfileplaceeditdialog.cpp

To: shubham, ngraham, broulik, #dolphin, #frameworks, dfaure, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

pino requested changes to this revision.
pino added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kfileplaceeditdialog.cpp:66
> +// Get icon except for trash
> +if (url.scheme() != QLatin1String("trash")) {
> +icon = dialog->icon();

This duplicates the logic that is used in the constructor below. A better 
option might be to create an helper class method to get whether the icon can be 
edited, basically checking for `m_iconButton`.

> kfileplaceeditdialog.cpp:123
>  
>  whatsThisText = i18n("This is the icon that will appear in the 
> Places panel."
>   "Click on the button to select a different 
> icon.");

This variable assignment can be moved inside the `if` as well, since this 
"what's this" text is used only for the icon button & its label.

> kfileplaceeditdialog.cpp:132
> +m_iconButton->setIconType(KIconLoader::NoGroup, KIconLoader::Place);
> +
> +if (icon.isEmpty()) {

Extra empty line.

> kfileplaceeditdialog.cpp:138
> +}
> +
> +m_iconButton->setWhatsThis(whatsThisText);

Extra empty line.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, dfaure, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham added a comment.


  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?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, dfaure, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

pino added a comment.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, dfaure, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham added a comment.


  indirectly you mean to transfer all the logic to helper function

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, dfaure, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

pino added a comment.


  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()`.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, dfaure, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, dfaure, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

pino added a comment.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, dfaure, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham added a comment.


  okay, then it's fine

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, dfaure, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham updated this revision to Diff 38637.
shubham added a comment.


  Add getter for icon's editability

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14360?vs=38633&id=38637

REVISION DETAIL
  https://phabricator.kde.org/D14360

AFFECTED FILES
  src/filewidgets/kfileplaceeditdialog.cpp
  src/filewidgets/kfileplaceeditdialog.h

To: shubham, ngraham, broulik, #dolphin, #frameworks, dfaure, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham removed a reviewer: dfaure.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino, dfaure
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham added a comment.


  ping?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


  This works and doesn't crash Gwenview for me anymore, yay! Please fix the 
below issues. And @pino, does the code look sensible now?

INLINE COMMENTS

> kfileplaceeditdialog.h:124
>  bool applicationLocal() const;
> +
> +/**

Remove the extra four spaces on this line please

> kfileplaceeditdialog.h:127
> + * @returns check for icon's editabilty
> + */
> +bool canEditIcon() const;

This is a public function so please add `@since 5.49`

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> kfileplaceeditdialog.h:126
> +/**
> + * @returns check for icon's editabilty
> + */

Change the text to "@returns whether the item's icon is editable"

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kfileplaceeditdialog.h:128
> + */
> +bool canEditIcon() const;
>  

Why is this a public API function? If it needs to be public, it should have 
better documentation. There is no 'setIconEditable()' function, so a hint why 
some icons are not editable would be nice.

Also, I would have named it 'isIconEditable()'.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham updated this revision to Diff 38824.
shubham added a comment.


  made above requested changes

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14360?vs=38637&id=38824

REVISION DETAIL
  https://phabricator.kde.org/D14360

AFFECTED FILES
  src/filewidgets/kfileplaceeditdialog.cpp
  src/filewidgets/kfileplaceeditdialog.h

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham added a comment.


  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 way to describe it in less space, any suggestions

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Nathaniel Graham]

ngraham added a comment.


  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)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Christoph Feck]

cfeck added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham added a comment.


  @cfeck agree with you(it's just a helper)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Nathaniel Graham]

ngraham added a comment.


  Yep, I agree.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham updated this revision to Diff 38852.
shubham added a comment.


  Remove helper function isIconEditable() (wasn't required), the check can 
directly be carried out in if clause for dialog->m_iconButton

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14360?vs=38824&id=38852

REVISION DETAIL
  https://phabricator.kde.org/D14360

AFFECTED FILES
  src/filewidgets/kfileplaceeditdialog.cpp

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

pino added a comment.


  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.

INLINE COMMENTS

> kfileplaceeditdialog.cpp:65
>  label   = dialog->label();
> -icon= dialog->icon();
> +if (dialog->m_iconButton) {
> +icon = dialog->icon();

This is even worse than the method. Please switch back to the method, since it 
is the better solution,

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

pino requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham updated this revision to Diff 38868.
shubham added a comment.


  revert back

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14360?vs=38852&id=38868

REVISION DETAIL
  https://phabricator.kde.org/D14360

AFFECTED FILES
  src/filewidgets/kfileplaceeditdialog.cpp
  src/filewidgets/kfileplaceeditdialog.h

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Christoph Feck]

cfeck added a comment.


  If it's not public, it doesn't need a `@since` tag.
  
  Why isn't the header called *_p.h ...?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

pino added a comment.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham added a comment.


  ping?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> kfileplaceeditdialog.cpp:220
> +{
> +return m_iconButton;
> +}

This will produce error on strict level it should be

  m_iconButton != nullptr

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: anthonyfieroni, cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: anthonyfieroni, cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham updated this revision to Diff 38913.
shubham added a comment.


  remove @since tag,
  return m_iconButton  != nullptr

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14360?vs=38868&id=38913

REVISION DETAIL
  https://phabricator.kde.org/D14360

AFFECTED FILES
  src/filewidgets/kfileplaceeditdialog.cpp
  src/filewidgets/kfileplaceeditdialog.h

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: anthonyfieroni, cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in kfileplaceeditdialog.cpp:220
> This will produce error on strict level it should be
> 
>   m_iconButton != nullptr

Which strict level is that? AFAIK it's perfectly valid to "cast" a pointer into 
a boolean, we do this *everywhere*...

> kfileplaceeditdialog.h:120
> + * @returns whether the item's icon is editable, beacause all icons are 
> not 
> + * (e.g. the Trash can display two icons, representing it's full and 
> empty states
> + * it's simpler to make these icons non-editable than to provide an 
> interface to edit them both)

it's => its

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: dfaure, anthonyfieroni, cfeck, pino, kde-frameworks-devel, michaelh, 
ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham added a comment.


  @dfaure is this patch okay

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: dfaure, anthonyfieroni, cfeck, pino, kde-frameworks-devel, michaelh, 
ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham updated this revision to Diff 38927.
shubham added a comment.


  Fix typos
  return m_iconButton

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14360?vs=38913&id=38927

REVISION DETAIL
  https://phabricator.kde.org/D14360

AFFECTED FILES
  src/filewidgets/kfileplaceeditdialog.cpp
  src/filewidgets/kfileplaceeditdialog.h

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: dfaure, anthonyfieroni, cfeck, pino, kde-frameworks-devel, michaelh, 
ngraham, bruns

D14360: Remove custom icon selection for trash

[David Faure]

dfaure accepted this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino, dfaure
Cc: dfaure, anthonyfieroni, cfeck, pino, kde-frameworks-devel, michaelh, 
ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham added a comment.


  @dfaure I don't have commit access

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino, dfaure
Cc: dfaure, anthonyfieroni, cfeck, pino, kde-frameworks-devel, michaelh, 
ngraham, bruns

D14360: Remove custom icon selection for trash

[Christoph Feck]

cfeck accepted this revision.
cfeck added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino, dfaure, cfeck
Cc: dfaure, anthonyfieroni, cfeck, pino, kde-frameworks-devel, michaelh, 
ngraham, bruns

D14360: Remove custom icon selection for trash

[Christoph Feck]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:aa9b63b45105: Remove custom icon selection for trash 
(authored by shubham, committed by cfeck).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D14360?vs=38927&id=38939#toc

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14360?vs=38927&id=38939

REVISION DETAIL
  https://phabricator.kde.org/D14360

AFFECTED FILES
  src/filewidgets/kfileplaceeditdialog.cpp
  src/filewidgets/kfileplaceeditdialog.h

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino, dfaure, cfeck
Cc: dfaure, anthonyfieroni, cfeck, pino, kde-frameworks-devel, michaelh, 
ngraham, bruns

D14360: Remove custom icon selection for trash

[Christoph Feck]

cfeck added a comment.


  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?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino, dfaure, cfeck
Cc: dfaure, anthonyfieroni, cfeck, pino, kde-frameworks-devel, michaelh, 
ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham added a comment.


  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?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino, dfaure, cfeck
Cc: dfaure, anthonyfieroni, cfeck, pino, kde-frameworks-devel, michaelh, 
ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham added a comment.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino, dfaure, cfeck
Cc: dfaure, anthonyfieroni, cfeck, pino, kde-frameworks-devel, michaelh, 
ngraham, bruns

D14360: Remove custom icon selection for trash

[Christoph Feck]

cfeck added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino, dfaure, cfeck
Cc: dfaure, anthonyfieroni, cfeck, pino, kde-frameworks-devel, michaelh, 
ngraham, bruns

D14360: Remove custom icon selection for trash

[Shubham]

shubham added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino, dfaure, cfeck
Cc: dfaure, anthonyfieroni, cfeck, pino, kde-frameworks-devel, michaelh, 
ngraham, bruns

D14360: Remove custom icon selection for trash

[David Faure]

dfaure added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14360

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino, dfaure, cfeck
Cc: dfaure, anthonyfieroni, cfeck, pino, kde-frameworks-devel, michaelh, 
ngraham, bruns