D14360: Remove custom icon selection for trash

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-framewor

D14360: Remove custom icon selection for trash

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

D14360: Remove custom icon selection for trash

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 h

D14360: Remove custom icon selection for trash

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

D14360: Remove custom icon selection for trash

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 acc

D14360: Remove custom icon selection for trash

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

D14360: Remove custom icon selection for trash

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 COMMI

D14360: Remove custom icon selection for trash

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

D14360: Remove custom icon selection for trash

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

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 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/

D14360: Remove custom icon selection for trash

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

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 *every

D14360: Remove custom icon selection for trash

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 FIL

D14360: Remove custom icon selection for trash

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,

D14360: Remove custom icon selection for trash

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, ng

D14360: Remove custom icon selection for trash

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

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

D14360: Remove custom icon selection for trash

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,

D14360: Remove custom icon selection for trash

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

D14360: Remove custom icon selection for trash

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 whi

D14360: Remove custom icon selection for trash

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

D14360: Remove custom icon selection for trash

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

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 n

D14360: Remove custom icon selection for trash

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 emp

D14360: Remove custom icon selection for trash

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.

D14360: Remove custom icon selection for trash

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

D14360: Remove custom icon selection for trash

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/kfi

D14360: Remove custom icon selection for trash

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

D14360: Remove custom icon selection for trash

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

D14360: Remove custom icon selection for trash

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

D14360: Remove custom icon selection for trash

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

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 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/filewidget

D14360: Remove custom icon selection for trash

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

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

D14360: Remove custom icon selection for trash

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

D14360: Remove custom icon selection for trash

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_iconButt

D14360: Remove custom icon selection for trash

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 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?

D14360: Remove custom icon selection for trash

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

D14360: Remove custom icon selection for trash

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

D14360: Remove custom icon selection for trash

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 AFFEC

D14360: Remove custom icon selection for trash

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

D14360: Remove custom icon selection for trash

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

D14360: Remove custom icon selection for trash

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 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, br

D14360: Remove custom icon selection for trash

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 poi

D14360: Remove custom icon selection for trash

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,

D14360: Remove custom icon selection for trash

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, l

D14360: Remove custom icon selection for trash

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, #dol

D14360: Remove custom icon selection for trash

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 rea

D14360: Remove custom icon selection for trash

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, #dol

D14360: Remove custom icon selection for trash

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() !=

D14360: Remove custom icon selection for trash

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 t

D14360: Remove custom icon selection for trash

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

D14360: Remove custom icon selection for trash

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

D14360: Remove custom icon selection for trash

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, ng

D14360: Remove custom icon selection for trash

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 `KFilePl

D14360: Remove custom icon selection for trash

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