KIO: try to assign an icon to action submenus

[Chloe Kudryavtsev]

On 3/15/20 12:19 PM, David Faure wrote:
> On vendredi 13 mars 2020 21:19:42 CET Chloe Kudryavtsev wrote:
>> On 3/13/20 2:18 PM, David Faure wrote:
>>> On mardi 3 mars 2020 18:29:47 CET Chloe Kudryavtsev wrote:
>>>> Currently, action submenus (X-KDE-Submenu) have no icons.
>>>> This patch makes it inherit the icon of the first action.
>>>
>>> I wonder if this is always wanted? The icon for the first action might not
>>> be representative of what the other actions do.
>>>
>>> Wouldn't it be better to be able to explicitly specify the icon for the
>>> submenu?
>>
>> That would be better, but that can't easily be done without reorganizing
>> how the whole thing works.
>> insertServicesSubmenus() currently receives a QMap<QString,
>> ServiceList>, the target QMenu and a boolean.
>> Meanwhile, the icon for the desktop file as a whole would be available
>> in the KConfigGroup (from desktopFile).
>>
>> Basically, making the "global" (i.e [Desktop Entry]) Icon setting be
>> what is used would require breaking API (changing the signature of
>> insertServicesSubmenus).
> 
> If you're talking about KFileItemActionsPrivate::insertServicesSubmenus
> that's a method of the Private class (as the classname indicates),
> so you can change it as much as you want, it's not part of the public API.
> 
>> In the meanwhile, in practice, ServiceMenus do have (at the very least)
>> similar icons, because TryExec is effectively global (there is no
>> support for disabling individual actions, it's done purely on a
>> per-desktop-file basis).
> 
> Right, but I don't like putting something in place with limitations, that 
> someone else will surely have a problem with, one day.


Sorry, it appears I wasn't sufficiently clear, so let's walk through it.
insertServicesSubmenus receives a QMap<QString, ServiceList>.
ServiceList is a QList<KServiceAction>.
Reading back I'm noticing that I claimed it's only take changing the
signature, and assume I was talking about the QMap<QString, ServiceList>
bit, but it's my fault regardless.

insertServicesSubmenus will iterate over the values in the QMap (i.e
each submenu in a given category) - if you pass an icon as a call to
insertServicesSubmenus, *every submenu per category will have the same
icon*.
The categories in question are userPriority, user and userTopLevel - i.e
entirely separate submenus from different .desktop files.

So we need to be able to pass an icon inside of a ServiceList.
As mentioned, ServiceList is a QList<KServiceAction>.
QList is part of Qt and KServiceAction is part of KService.

If we wanted to approach it in this way, then, the best way would be to
extend KService, and add a parent() pointer or similar to a KService -
which would have its own .icon() call.

The ServiceLists in question are accessed via PopupServices::selectList,
bound to a local ServiceList and then populated.
As a consequence, we know exactly where they're coming from:
KDesktopFileActions::userDefinedServices.
All 3 versions of that function eventually boil down to the one with a
KService variable, meaning it would be possible to add this information
at that level, though some care would be needed for it to not be freed
(either through smart pointers, or by allocating it on the heap).

Alternatively, we could change the type of ServiceList to something that
wraps around (or inherits) QList, but still has an icon() method.
KDesktopFileActions::userDefinedServices would then need to return a
ServiceList after populating it.
The issue here is that this would change public API
(userDefinedServices, which would now return a ServiceList rather than a
raw QList<KServiceAction>) and require making ServiceList more
standalone/available.

Finally, we could take the above approach, but make a new-ified
ServiceList from the output of userDefinedServices.
This is fairly simple in the final invocation, where *it2 is the
KService in question.
In the other two scenarios, we have a KDesktopFile.
We could use KDesktopFile::readIcon() to do the same process.
The issue with this approach is that it would be extremely messy.
Extending types in Qt is non-trivial, and if we wrap around QList
basically every line that so much as mentions ServiceList would need to
be modified (including things like assignment through a reference).
This can be done, but I'm not particularly convinced this is an improvement.
SubMenus are already used fairly rarely (being a KDE-specific key), and
none of the .desktop ServiceMenus present on my system (nor that I'm
aware of) even use this feature (possibly due to the lack of icon support).

>> I would participate in an effort to extend KConfig, KIO and co. to
>> improve the overall handling of .desktop files, but the process in
>> general is kind of a bother, and I don't really like touching C++
>> nowadays either, so it's a hard sell to put that as a high priority
>> for me.
>
> This seems a rather strong statement for the work at hand here.
> If all that's needed is to pass an icon to insertServicesSubmenus you
> surely don't need to extend KConfig...

Same story here, apparently - sorry for not being sufficiently clear.
When I'm talking about extending KConfig, I'm talking about the other
work I would have considered doing (enabling per-action TryExec handling).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0xEB24BAD412DE2823.asc
Type: application/pgp-keys
Size: 884 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200315/99b505b3/attachment.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: OpenPGP digital signature
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200315/99b505b3/attachment.sig>