Re: KIO: try to assign an icon to action submenus
On dimanche 15 mars 2020 19:11:39 CET Chloe Kudryavtsev wrote:
> So we need to be able to pass an icon inside of a ServiceList.
> As mentioned, ServiceList is a QList.
> QList is part of Qt and KServiceAction is part of KService.
The usual solution for this is to replace ServiceList with
struct SubMenu
{
QList actions;
QIcon icon;
};
in the code that is about a submenu, like in insertServicesSubmenus.
But I see what you mean, submenus are an aggregation of independent actions
who belong to the same priority (as selected by selectList()), so there's no
single definition for those submenus, where we would be able to set an icon.
C++ problems can be fixed, but not conceptual ones like the one above.
I'm dropping my objections.
https://phabricator.kde.org/D27950 approved.
--
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5
Re: KIO: try to assign an icon to action submenus
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> 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. ServiceList is a QList. Reading back I'm noticing that I claimed it's only take changing the signature, and assume I was talking about the QMap 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. 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) 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
Re: KIO: try to assign an icon to action submenus
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 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. > 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... -- David Faure, fa...@kde.org, http://www.davidfaure.fr Working on KDE Frameworks 5
Re: KIO: try to assign an icon to action submenus
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, 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). 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). Ideally, of course, everything would be explicit, but the amount of changes quickly grows if one wants to do this right. 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. - 0xEB24BAD412DE2823.asc Description: application/pgp-keys signature.asc Description: OpenPGP digital signature
Re: KIO: try to assign an icon to action submenus
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? -- David Faure, fa...@kde.org, http://www.davidfaure.fr Working on KDE Frameworks 5
Re: KIO: try to assign an icon to action submenus
Hi Chloe, I just submitted the patch on your behalf: https://phabricator.kde.org/D27950 Built it and seems to work. Hope this helps. Aleix On Sat, Mar 7, 2020 at 12:27 AM Chloe Kudryavtsev wrote: > > Currently, action submenus (X-KDE-Submenu) have no icons. > This patch makes it inherit the icon of the first action. > There will always be at least one action, since empty submenus are > eliminated earlier in the function. > If the icon of the first action is empty, it will gracefully revert to > old behavior (not having an icon). > > Please find the single-line patch attached (I have little desire to make > yet another account in phabricator, especially for such a small patch, > and was encouraged to send the patch via email on irc).
KIO: try to assign an icon to action submenus
Currently, action submenus (X-KDE-Submenu) have no icons.
This patch makes it inherit the icon of the first action.
There will always be at least one action, since empty submenus are
eliminated earlier in the function.
If the icon of the first action is empty, it will gracefully revert to
old behavior (not having an icon).
Please find the single-line patch attached (I have little desire to make
yet another account in phabricator, especially for such a small patch,
and was encouraged to send the patch via email on irc).
diff --git a/src/widgets/kfileitemactions.cpp b/src/widgets/kfileitemactions.cpp
index f5f4a55a..f487002a 100644
--- a/src/widgets/kfileitemactions.cpp
+++ b/src/widgets/kfileitemactions.cpp
@@ -165,6 +165,7 @@ int KFileItemActionsPrivate::insertServicesSubmenus(const QMapsetTitle(it.key());
actionSubmenu->menuAction()->setObjectName(QStringLiteral("services_submenu")); // for the unittest
+actionSubmenu->setIcon(QIcon::fromTheme(it.value().first().icon()))
menu->addMenu(actionSubmenu);
count += insertServices(it.value(), actionSubmenu, isBuiltin);
}
0xEB24BAD412DE2823.asc
Description: application/pgp-keys
signature.asc
Description: OpenPGP digital signature
