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

2020-01-21 Thread Méven Car 
meven added a task: T12551: Revamp KCM/Component chooser.

REPOSITORY
  R309 KService

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

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-21 Thread Méven Car 
This revision was automatically updated to reflect the committed changes.
Closed by commit R309:f10db4e6f0af: Make Default Applications in 
mimeapps.list the preferred applications (authored by meven).

REPOSITORY
  R309 KService

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26690?vs=74002=74003

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

AFFECTED FILES
  autotests/kmimeassociationstest.cpp
  src/kdeinit/ktoolinvocation_x11.cpp
  src/sycoca/kmimeassociations.cpp

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-21 Thread Méven Car 
meven updated this revision to Diff 74002.
meven added a comment.


  Fix indentation issue

REPOSITORY
  R309 KService

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26690?vs=73996=74002

BRANCH
  arcpatch-D26690

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

AFFECTED FILES
  autotests/kmimeassociationstest.cpp
  src/kdeinit/ktoolinvocation_x11.cpp
  src/sycoca/kmimeassociations.cpp

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-21 Thread Kevin Ottens 
ervin accepted this revision.

REPOSITORY
  R309 KService

BRANCH
  arcpatch-D26690

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

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-21 Thread Méven Car 
meven marked 6 inline comments as done.

REPOSITORY
  R309 KService

BRANCH
  arcpatch-D26690

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

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-21 Thread Méven Car 
meven updated this revision to Diff 73996.
meven added a comment.


  Add explanatory comment regarding 25 magic number

REPOSITORY
  R309 KService

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26690?vs=73913=73996

BRANCH
  arcpatch-D26690

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

AFFECTED FILES
  autotests/kmimeassociationstest.cpp
  src/kdeinit/ktoolinvocation_x11.cpp
  src/sycoca/kmimeassociations.cpp

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-20 Thread Kevin Ottens 
ervin added inline comments.

INLINE COMMENTS

> meven wrote in kmimeassociations.cpp:104
> The first 25 with the highest preferences cannot be affected but then they 
> can have the same base reference as the next most category loaded.
> Like the 26th "Default Applications" would have the same preference as the 
> first "Added Applications"
> But this is of low concern here anyway.
> It just shows the limitations of the current design based on integer ordering.
> 
> @ervin Do you still feel it needs more comment ?

The first one was fine, but what I meant was a comment in the code for when 
people bump into those magic numbers. ;-)

REPOSITORY
  R309 KService

BRANCH
  arcpatch-D26690

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

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-20 Thread Méven Car 
meven added inline comments.

INLINE COMMENTS

> dfaure wrote in kmimeassociations.cpp:104
> More precisely, it assumes that a single mimetype entry doesn't have more 
> than 25 desktop files associated with it. And even then, what would happen is 
> that the ordering is wrong after the first 25. I'd say after 25 you start to 
> not really care about the order

The first 25 with the highest preferences cannot be affected but then they can 
have the same base reference as the next most category loaded.
Like the 26th "Default Applications" would have the same preference as the 
first "Added Applications"
But this is of low concern here anyway.
It just shows the limitations of the current design based on integer ordering.

@ervin Do you still feel it needs more comment ?

REPOSITORY
  R309 KService

BRANCH
  arcpatch-D26690

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

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-20 Thread David Faure 
dfaure added inline comments.

INLINE COMMENTS

> meven wrote in kmimeassociations.cpp:104
> The code kinda assumes we don't have more than 25 entries by configgroup.

More precisely, it assumes that a single mimetype entry doesn't have more than 
25 desktop files associated with it. And even then, what would happen is that 
the ordering is wrong after the first 25. I'd say after 25 you start to not 
really care about the order

REPOSITORY
  R309 KService

BRANCH
  arcpatch-D26690

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

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-20 Thread Méven Car 
meven added inline comments.

INLINE COMMENTS

> dfaure wrote in kmimeassociations.cpp:104
> No. What about my job security? ;-)
> 
> It's half of the +50 from line 86.
> 
> The range 1000-1025 is used by Added Associations, the range 1025-1050 is 
> used by Default Applications, and so on for the next file.

The code kinda assumes we don't have more than 25 entries by configgroup.

REPOSITORY
  R309 KService

BRANCH
  arcpatch-D26690

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

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-20 Thread David Faure 
dfaure added inline comments.

INLINE COMMENTS

> ervin wrote in kmimeassociations.cpp:104
> Shouldn't the comment also include why the value 25 (and not 1, or 42 or NaN 
> ;-)) for mere mortals like me?

No. What about my job security? ;-)

It's half of the +50 from line 86.

The range 1000-1025 is used by Added Associations, the range 1025-1050 is used 
by Default Applications, and so on for the next file.

REPOSITORY
  R309 KService

BRANCH
  arcpatch-D26690

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

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-20 Thread Kevin Ottens 
ervin added inline comments.

INLINE COMMENTS

> kmimeassociations.cpp:104
> +// Other than that, they work the same...
> +parseAddedAssociations(KConfigGroup(, "Default Applications"), 
> file, basePreference + 25);
>  }

Shouldn't the comment also include why the value 25 (and not 1, or 42 or NaN 
;-)) for mere mortals like me?

REPOSITORY
  R309 KService

BRANCH
  arcpatch-D26690

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

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-20 Thread Méven Car 
meven edited the summary of this revision.

REPOSITORY
  R309 KService

BRANCH
  arcpatch-D26690

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

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-20 Thread David Faure 
dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  Good solution :-) LOL.

REPOSITORY
  R309 KService

BRANCH
  arcpatch-D26690

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

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-20 Thread Méven Car 
meven updated this revision to Diff 73913.
meven added a comment.


  Simplify implementation of Default Applications parsing, update tests

REPOSITORY
  R309 KService

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26690?vs=73781=73913

BRANCH
  arcpatch-D26690

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

AFFECTED FILES
  autotests/kmimeassociationstest.cpp
  src/kdeinit/ktoolinvocation_x11.cpp
  src/sycoca/kmimeassociations.cpp

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-18 Thread Méven Car 
meven planned changes to this revision.
meven added a comment.


  In D26690#596551 , @dfaure wrote:
  
  > I'm quite confused by all this. Wouldn't it be enough to do 
http://www.davidfaure.fr/2020/kmimeassociations.cpp.diff ?
  >  The modified unittest passes :-)
  
  
  No because, `Default Applications` containst lists, means a same mime type in 
this group may have several
  
  In D26690#596551 , @dfaure wrote:
  
  > I'm quite confused by all this. Wouldn't it be enough to do 
http://www.davidfaure.fr/2020/kmimeassociations.cpp.diff ?
  >  The modified unittest passes :-)
  
  
  Well I think it does the trick.
  Simply by adding 25 to the basePreferrence is way more simple.
  
  I will integrate this to this patch as well as the unit test.
  Thanks

REPOSITORY
  R309 KService

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

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-18 Thread David Faure 
dfaure added a comment.


  I'm quite confused by all this. Wouldn't it be enough to do 
http://www.davidfaure.fr/2020/kmimeassociations.cpp.diff ?
  The modified unittest passes :-)

REPOSITORY
  R309 KService

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

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-17 Thread Méven Car 
meven edited the summary of this revision.

REPOSITORY
  R309 KService

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

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-17 Thread Méven Car 
meven added a comment.


  In D26690#596116 , @ngraham wrote:
  
  > Does this fix https://bugs.kde.org/show_bug.cgi?id=403499?
  
  
  Partially I believe, D26557  should have 
helped too.
  Currently a file change of mimeapps.list does not trigger kbuildsyscoca 
AFAICT so manual file edition will cause issues.
  And D26731  will help to use the 
mimeapps.list defined browser before the kdeglobals one.

REPOSITORY
  R309 KService

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

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-17 Thread Nathaniel Graham 
ngraham added a comment.


  Does this fix https://bugs.kde.org/show_bug.cgi?id=403499?

REPOSITORY
  R309 KService

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

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

2020-01-17 Thread Méven Car 
meven updated this revision to Diff 73781.
meven added a comment.


  KCM/Component Revamp browser config

REPOSITORY
  R309 KService

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26690?vs=73778=73781

BRANCH
  master

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

AFFECTED FILES
  src/kdeinit/ktoolinvocation_x11.cpp
  src/sycoca/kmimeassociations.cpp
  src/sycoca/kmimeassociations_p.h

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

2020-01-17 Thread Méven Car 
meven updated this revision to Diff 73778.
meven marked 4 inline comments as done.
meven added a comment.


  Review : Add an enum AddServiceFlag

REPOSITORY
  R309 KService

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26690?vs=73679=73778

BRANCH
  master

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

AFFECTED FILES
  src/kdeinit/ktoolinvocation_x11.cpp
  src/sycoca/kmimeassociations.cpp
  src/sycoca/kmimeassociations_p.h

To: meven, dfaure, dvratil, ervin, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

2020-01-16 Thread 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 , const 
> KServiceOffer )
> +{

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

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

2020-01-16 Thread Méven Car 
meven updated this revision to Diff 73679.
meven added a comment.


  Update code to have a better implementation reusing existant API

REPOSITORY
  R309 KService

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26690?vs=73633=73679

BRANCH
  master

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

AFFECTED FILES
  src/kdeinit/ktoolinvocation_x11.cpp
  src/sycoca/kmimeassociations.cpp
  src/sycoca/kmimeassociations_p.h

To: meven, dfaure, dvratil, ervin
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

2020-01-16 Thread Méven Car 
meven added a reviewer: Frameworks.

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

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

2020-01-16 Thread Méven Car 
meven retitled this revision from "[WIP] Add 
KMimeTypeTrader::defaultSchemaHandler to return default service associated with 
scheme" to "Make "Default Applications" in mimeapps.list the preferred 
applications".
meven edited the summary of this revision.

REPOSITORY
  R309 KService

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

To: meven, dfaure, dvratil, ervin
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns