D13549: Switch KIO::convertSize() to KFormat::formatByteSize()

2018-06-24 Thread Pino Toscano
pino added a comment. Polite ping. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D13549 To: pino, dfaure Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13670: Add subseq operator to match sub-sequences

2018-06-24 Thread Michael Eden
michaeleden added a comment. @dfaure if a user was trying to find `LibreOffice 6.0 Writer` with `subin` they would have type `libre` to narrow it down to LibreOffice programs, then `libreoffice 6.0 writer` to get to that application. with `subseq` they can type `libre` to get to LibreOf

KDE CI: Frameworks knewstuff kf5-qt5 FreeBSDQt5.10 - Build # 12 - Unstable!

2018-06-24 Thread CI System
BUILD UNSTABLE Build URL https://build.kde.org/job/Frameworks%20knewstuff%20kf5-qt5%20FreeBSDQt5.10/12/ Project: Frameworks knewstuff kf5-qt5 FreeBSDQt5.10 Date of build: Mon, 25 Jun 2018 03:23:53 + Build duration: 1 min 49 sec and counting JUnit Tests

D13706: [KMoreTools] Enable installing tools via appstream url

2018-06-24 Thread Nathaniel Graham
ngraham added a comment. @gregormi > Mostly, desktopEntryName and appstreamComponentId are equal. So, maybe one could use a placeholder (e.g. ~) to reuse the desktopEntryName Keep in mind that AppStream IDs are not guessable. Some apps use the correct format, e.g. "org.kde.konsole",

D13706: [KMoreTools] Enable installing tools via appstream url

2018-06-24 Thread gregormi
gregormi accepted this revision. gregormi added a comment. Ok from my side. Probably Dominik also wants to approve. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D13706 To: nicolasfella, #frameworks, gregormi, dhaumann Cc: dhaumann, kde-frameworks-devel, michaelh,

D13706: [KMoreTools] Enable installing tools via appstream url

2018-06-24 Thread Nicolas Fella
nicolasfella added inline comments. INLINE COMMENTS > nicolasfella wrote in kmoretools_p.h:394 > + would give the same result. What % does is acting as a QStringBuilder, > which is more performant in theory (not that it really matters in this case) See http://doc.qt.io/qt-5/qstring.html#more-ef

D13706: [KMoreTools] Enable installing tools via appstream url

2018-06-24 Thread Nicolas Fella
nicolasfella added inline comments. INLINE COMMENTS > gregormi wrote in kmoretools_p.h:394 > What does the % sign do here? Can this be used to concatenate strings? Did > not try it myself. > > Otherwise ready to land. + would give the same result. What % does is acting as a QStringBuilder, whi

D13706: [KMoreTools] Enable installing tools via appstream url

2018-06-24 Thread gregormi
gregormi added inline comments. INLINE COMMENTS > kmoretools_p.h:394 > + > +QUrl appstreamUrl = QUrl(QStringLiteral("appstream://") % > appstreamId); > + What does the % sign do here? Can this be used to concatenate strings? Did not try it myself. Otherwise ready to land. REPOSITORY

D13706: [KMoreTools] Enable installing tools via appstream url

2018-06-24 Thread Nicolas Fella
nicolasfella updated this revision to Diff 36620. nicolasfella added a comment. - Replace appstreamUrl with appstreamId REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13706?vs=36617&id=36620 BRANCH appstream REVISION DETAIL https://phabricator.kde.o

D13706: [KMoreTools] Enable installing tools via appstream url

2018-06-24 Thread gregormi
gregormi added inline comments. INLINE COMMENTS > kmoretoolspresets.cpp:59 > // > -ADD_ENTRY("angrysearch",0, > "https://github.com/DoTheEvo/ANGRYsearch";); > -ADD_ENTRY("com.uploadedlobster.peek", 0, > "https://github.com/phw/peek";); // easy to u

D13706: [KMoreTools] Enable installing tools via appstream url

2018-06-24 Thread gregormi
gregormi added inline comments. INLINE COMMENTS > kmoretools.h:500 > + */ > +void setAppstreamUrl(const QUrl& url); > + I am not so familiar with appstream. Why not only setting the COMPONENT-ID instead of the whole URL? Then the method would be named setAppstreamComponentId. REPOSITO

D13706: [KMoreTools] Enable installing tools via appstream url

2018-06-24 Thread Nicolas Fella
nicolasfella updated this revision to Diff 36617. nicolasfella added a comment. - Add documentation REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13706?vs=36612&id=36617 BRANCH appstream REVISION DETAIL https://phabricator.kde.org/D13706 AFFECTED

D13706: [KMoreTools] Enable installing tools via appstream url

2018-06-24 Thread Nicolas Fella
nicolasfella marked 2 inline comments as done. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D13706 To: nicolasfella, #frameworks, gregormi, dhaumann Cc: dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

2018-06-24 Thread Dominik Haumann
dhaumann requested changes to this revision. dhaumann added a comment. This revision now requires changes to proceed. Looks already pretty good to me, but please add API documentation. Rule of thumb: if you extend a header file that is documented, then follow this scheme and also document

D13706: [KMoreTools] Enable installing tools via appstream url

2018-06-24 Thread gregormi
gregormi added a comment. First of all, thanks for adding this feature. This was missing a long time :-). I will do some comments in the code. INLINE COMMENTS > kmoretools.h:488-491 > +QUrl appstreamUrl() const; > + > +void setAppstreamUrl(const QUrl& url); > + Please add a comment

D13706: [KMoreTools] Enable installing tools via appstream url

2018-06-24 Thread Dominik Haumann
dhaumann added a reviewer: gregormi. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D13706 To: nicolasfella, #frameworks, gregormi Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13698: Improve ECMAddAppIconMacro.

2018-06-24 Thread Vincent Pinon
vpinon added inline comments. INLINE COMMENTS > dschmidt wrote in FindIcoTool.cmake:1 > @vpinon Can you let me know what you want here? Hello, FindIcoTool.cmake is largely copied from FindPng2Ico.cmake, I'm even not sure I can really claim the copyright. In any case, I would choose the same lic

KDE CI: Frameworks kio kf5-qt5 SUSEQt5.9 - Build # 151 - Unstable!

2018-06-24 Thread CI System
BUILD UNSTABLE Build URL https://build.kde.org/job/Frameworks%20kio%20kf5-qt5%20SUSEQt5.9/151/ Project: Frameworks kio kf5-qt5 SUSEQt5.9 Date of build: Sun, 24 Jun 2018 16:53:57 + Build duration: 17 min and counting JUnit Tests Name: (root) Faile

KDE CI: Frameworks kio kf5-qt5 SUSEQt5.10 - Build # 302 - Still Unstable!

2018-06-24 Thread CI System
BUILD UNSTABLE Build URL https://build.kde.org/job/Frameworks%20kio%20kf5-qt5%20SUSEQt5.10/302/ Project: Frameworks kio kf5-qt5 SUSEQt5.10 Date of build: Sun, 24 Jun 2018 16:53:57 + Build duration: 6 min 44 sec and counting JUnit Tests Name: (roo

KDE CI: Frameworks kio kf5-qt5 FreeBSDQt5.10 - Build # 57 - Still Unstable!

2018-06-24 Thread CI System
BUILD UNSTABLE Build URL https://build.kde.org/job/Frameworks%20kio%20kf5-qt5%20FreeBSDQt5.10/57/ Project: Frameworks kio kf5-qt5 FreeBSDQt5.10 Date of build: Sun, 24 Jun 2018 16:53:57 + Build duration: 4 min 49 sec and counting JUnit Tests Name:

D13706: [KMoreTools] Enable installing tools via appstream url

2018-06-24 Thread Nicolas Fella
nicolasfella updated this revision to Diff 36612. nicolasfella added a comment. - Add kmousetool url REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13706?vs=36609&id=36612 BRANCH appstream REVISION DETAIL https://phabricator.kde.org/D13706 AFFECTED

D13674: Make it possible to go up to root again

2018-06-24 Thread Jaime Torres Amate
This revision was automatically updated to reflect the committed changes. Closed by commit R241:1cea9463f471: Make it possible to go up to root again (authored by jtamate). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13674?vs=36534&id=36611 REVISION DETAIL h

D13706: [KMoreTools] Enable installing tools via appstream url

2018-06-24 Thread Nicolas Fella
nicolasfella updated this revision to Diff 36609. nicolasfella added a comment. - Add more appstream urls REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13706?vs=36606&id=36609 BRANCH appstream REVISION DETAIL https://phabricator.kde.org/D13706 AFF

D13706: [KMoreTools] Enable installing tools via appstream url

2018-06-24 Thread Nicolas Fella
nicolasfella edited the summary of this revision. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D13706 To: nicolasfella, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

2018-06-24 Thread Nicolas Fella
nicolasfella updated this revision to Diff 36606. nicolasfella added a comment. - Whitespace REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13706?vs=36604&id=36606 BRANCH appstream REVISION DETAIL https://phabricator.kde.org/D13706 AFFECTED FILES

D13706: [KMoreTools] Enable installing tools via appstream url

2018-06-24 Thread Nicolas Fella
nicolasfella created this revision. nicolasfella added a reviewer: Frameworks. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: kde-frameworks-devel. nicolasfella requested review of this revision. REVISION SUMMARY When a tool is not installed add an

D13698: Improve ECMAddAppIconMacro.

2018-06-24 Thread Dominik Schmidt
dschmidt marked an inline comment as done. dschmidt added inline comments. INLINE COMMENTS > cgiboudeaux wrote in FindIcoTool.cmake:1 > - Missing doc > - Missing license @vpinon Can you let me know what you want here? > cgiboudeaux wrote in ECMAddAppIcon.cmake:105-106 > ECMAddAppIcon has an uni

D13627: [KIconThemes] Isolate private data from race conditions

2018-06-24 Thread David Faure
dfaure added a comment. 1. write a unittest for the use case you're trying to solve 2. let's discuss if the use case is valid 3. make the correct, minimal fix, for that use case REPOSITORY R302 KIconThemes REVISION DETAIL https://phabricator.kde.org/D13627 To: anthonyfieroni, davided

D13674: Make it possible to go up to root again

2018-06-24 Thread David Faure
dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D13674 To: jtamate, dfaure, #frameworks Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D13670: Add subseq operator to match sub-sequences

2018-06-24 Thread David Faure
dfaure added a comment. I'm confused, what's the difference with subin? REPOSITORY R309 KService REVISION DETAIL https://phabricator.kde.org/D13670 To: michaeleden, dfaure, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13700: implement reading of the replaygain tags

2018-06-24 Thread Alexander Stippich
astippich added a comment. In D13700#282253 , @bruns wrote: > "dB" is nothing more than deci-Bel, i.e. 0.1 Bel. You can convert the value to e.g. mB or uB. This looks unfamiliar, but is e.g. also used by the CRDA WiFi compliance tool. I