BUG: 391200
Details
- Reviewers
ngraham broulik pino dfaure cfeck - Group Reviewers
Dolphin Frameworks - Commits
- R241:aa9b63b45105: Remove custom icon selection for trash
- Right click trash on places panel
- Edit
Result: No custom icon menu
Diff Detail
- Repository
- R241 KIO
- Lint
Lint Skipped - Unit
Unit Tests Skipped
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.
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.
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.
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
src/filewidgets/kfileplaceeditdialog.cpp | ||
---|---|---|
123 | No need to SCREAM in comments :) |
Check before getting the icon, whether it's editable or not
No need to get icon for trash
src/filewidgets/kfileplaceeditdialog.cpp | ||
---|---|---|
66 | 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. | |
123 | 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. | |
132 | Extra empty line. | |
138 | Extra empty line. |
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?)
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.
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).
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.
src/filewidgets/kfileplaceeditdialog.h | ||
---|---|---|
126 | Change the text to "@returns whether the item's icon is editable" |
src/filewidgets/kfileplaceeditdialog.h | ||
---|---|---|
128 | 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()'. |
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.
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.
Remove helper function isIconEditable() (wasn't required), the check can directly be carried out in if clause for dialog->m_iconButton
This is not a public API, since KFilePlaceEditDialog is a private class which is not even exported.
src/filewidgets/kfileplaceeditdialog.cpp | ||
---|---|---|
65 | This is even worse than the method. Please switch back to the method, since it is the better solution, |
If it's not public, it doesn't need a @since tag.
Why isn't the header called *_p.h ...?
No idea, it was not me to suggest that.
Why isn't the header called *_p.h ...?
No idea either.
src/filewidgets/kfileplaceeditdialog.cpp | ||
---|---|---|
220 | This will produce error on strict level it should be m_iconButton != nullptr |
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 :)
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?
@cfeck Thank you for committing it on my behalf. BTW can I give your name as supporter for KDE developer account application?
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.