D26690: Make "Default Applications" in mimeapps.list the preferred applications

[David Faure]

dfaure added a comment.


  Thanks for looking into this, I'm glad that finally someone does dig into this code.
  
  I'm a bit surprised by the solution though. The spec simply says
  
  - add any "Default Applications" and then "Added Associations" in the first mimeapps.list
  
  This doesn't need any reverse iteration or prepending, it's just about reading Default Applications before Added Associations rather than the other way around, isn't it?
  
  I think my comment 5 years ago also meant that the xdg spec allows for the default app (left-click in file manager) to be different from the preferred app (RMB / Open With).
  But looking at the spec now (Application ordering) it really treats "Default Applications" as higher-priority Added Associations, which proves that having separated the two is just complete nonsense, they serve the same purpose. Bleh. At least it's easier to implement this way :-)

INLINE COMMENTS

> kmimeassociations.cpp:112
> +        const QStringList services = group.readXdgListEntry(mimeName);
> +        // since the first has precendence and we prepend entries, we need to reverse the list of service
> +        std::reverse(services.begin(), services.end());

typo: precedence

> kmimeassociations.cpp:113
> +        // since the first has precendence and we prepend entries, we need to reverse the list of service
> +        std::reverse(services.begin(), services.end());
> +        const QString resolvedMimeName = mimeName.startsWith(QLatin1String("x-scheme-handler/")) ? mimeName : db.mimeTypeForName(mimeName).name();

Iterating with rbegin/rend (instead of the range-for on line 119) would be faster.

Or .... factorize the rest of the loop with the other method, since that's all duplicated otherwise? Then that's a good reason to keep the same range-for.

> kmimeassociations.cpp:125
> +                    //qDebug() << "prepending mime" << resolvedMimeName << "to service" << pService->entryPath() << "pref=" << pref;
> +                    m_offerHash.prependServiceOffer(resolvedMimeName, KServiceOffer(pService, pref, 0, pService->allowAsDefault()));
> +                    --pref;

Replace the last argument with true, clearly this service is allowed as default :)
(I'll remove allowAsDefault in KF6 anyway)

> kmimeassociations.cpp:200
>  
> +void KOfferHash::prependServiceOffer(const QString &serviceType, const KServiceOffer &offer)
> +{

I must be tired, but I don't see any difference between the code of this method and the one in addServiceOffer?

(and depending on what the difference should be, I think this should be a single method with an enum argument, to reduce duplication)

REPOSITORY
  R309 KService

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

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200116/8b4dc0a7/attachment-0001.html>