D13892: Restore compatibility of UDS::insert

2018-07-04 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/D13892

To: jtamate, dfaure, #frameworks, aacid
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13892: Restore compatibility of UDS::insert

2018-07-04 Thread Jaime Torres Amate
jtamate updated this revision to Diff 37170.
jtamate marked 2 inline comments as done.
jtamate added a comment.


  Added KIOCORE_DEPRECATED

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13892?vs=37163&id=37170

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

AFFECTED FILES
  src/core/udsentry.cpp
  src/core/udsentry.h

To: jtamate, dfaure, #frameworks, aacid
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


KDE CI: Frameworks purpose kf5-qt5 SUSEQt5.10 - Build # 68 - Unstable!

2018-07-04 Thread CI System
BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20purpose%20kf5-qt5%20SUSEQt5.10/68/
 Project:
Frameworks purpose kf5-qt5 SUSEQt5.10
 Date of build:
Thu, 05 Jul 2018 03:13:03 +
 Build duration:
2 min 42 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 2 test(s), Skipped: 0 test(s), Total: 3 test(s)Failed: TestSuite.alternativesmodeltest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report23%
(5/22)27%
(14/52)27%
(14/52)19%
(406/2095)17%
(186/1077)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(2/2)100%
(2/2)92%
(113/123)50%
(48/96)src100%
(8/8)100%
(8/8)68%
(212/312)50%
(104/210)src.externalprocess0%
(0/2)0%
(0/2)0%
(0/136)0%
(0/88)src.fileitemactionplugin0%
(0/1)0%
(0/1)0%
(0/17)0%
(0/12)src.plugins.email0%
(0/1)0%
(0/1)0%
(0/55)0%
(0/20)src.plugins.imgur0%
(0/2)0%
(0/2)0%
(0/186)0%
(0/69)src.plugins.kdeconnect0%
(0/1)0%
(0/1)0%
(0/32)0%
(0/12)src.plugins.ktp-sendfile0%
(0/1)0%
(0/1)0%
(0/28)0%
(0/12)src.plugins.nextcloud0%
(0/3)0%
(0/3)0%
(0/79)0%
(0/34)src.plugins.pastebin0%
(0/1)0%
(0/1)0%
(0/54)0%
(0/29)src.plugins.phabricator0%
(0/3)0%
(0/3)0%
(0/215)0%
(0/80)src.plugins.phabricator.quick0%
(0/5)0%
(0/5)0%
(0/82)0%
(0/57)src.plugins.phabricator.tests0%
(0/1)0%
(0/1)0%
(0/60)0%
(0/28)src.plugins.reviewboard0%
(0/3)0%
(0/3)0%
(0/233)0%
(0/76)src.plugins.reviewboard.quick0%
(0/7)0%
(0/7)0%
(0/153)0%
(0/92)src.plugins.saveas100%
(1/1)100%
(1/1)57%
(29/51)64%
(28/44)src.plugins.telegram0%
(0/1)0%
(0/1)0%
(0/48)0%
(0/22)src.plugins.youtube0%
(0/4)0%
(0/4)0%
(0/112)0%
   

KDE CI: Frameworks purpose kf5-qt5 SUSEQt5.9 - Build # 44 - Still Unstable!

2018-07-04 Thread CI System
BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20purpose%20kf5-qt5%20SUSEQt5.9/44/
 Project:
Frameworks purpose kf5-qt5 SUSEQt5.9
 Date of build:
Thu, 05 Jul 2018 03:13:03 +
 Build duration:
2 min 0 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 2 test(s), Skipped: 0 test(s), Total: 3 test(s)Failed: TestSuite.alternativesmodeltest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report23%
(5/22)27%
(14/52)27%
(14/52)20%
(429/2095)19%
(206/1077)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(2/2)100%
(2/2)92%
(113/123)50%
(48/96)src100%
(8/8)100%
(8/8)75%
(235/312)59%
(124/210)src.externalprocess0%
(0/2)0%
(0/2)0%
(0/136)0%
(0/88)src.fileitemactionplugin0%
(0/1)0%
(0/1)0%
(0/17)0%
(0/12)src.plugins.email0%
(0/1)0%
(0/1)0%
(0/55)0%
(0/20)src.plugins.imgur0%
(0/2)0%
(0/2)0%
(0/186)0%
(0/69)src.plugins.kdeconnect0%
(0/1)0%
(0/1)0%
(0/32)0%
(0/12)src.plugins.ktp-sendfile0%
(0/1)0%
(0/1)0%
(0/28)0%
(0/12)src.plugins.nextcloud0%
(0/3)0%
(0/3)0%
(0/79)0%
(0/34)src.plugins.pastebin0%
(0/1)0%
(0/1)0%
(0/54)0%
(0/29)src.plugins.phabricator0%
(0/3)0%
(0/3)0%
(0/215)0%
(0/80)src.plugins.phabricator.quick0%
(0/5)0%
(0/5)0%
(0/82)0%
(0/57)src.plugins.phabricator.tests0%
(0/1)0%
(0/1)0%
(0/60)0%
(0/28)src.plugins.reviewboard0%
(0/3)0%
(0/3)0%
(0/233)0%
(0/76)src.plugins.reviewboard.quick0%
(0/7)0%
(0/7)0%
(0/153)0%
(0/92)src.plugins.saveas100%
(1/1)100%
(1/1)57%
(29/51)64%
(28/44)src.plugins.telegram0%
(0/1)0%
(0/1)0%
(0/48)0%
(0/22)src.plugins.youtube0%
(0/4)0%
(0/4)0%
(0/112)0%
  

D13895: Fix off by one error in Cache::clear

2018-07-04 Thread David Edmundson
davidedmundson created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
davidedmundson requested review of this revision.

REVISION SUMMARY
  beginRemoveRows args are int first, int last
  
  i.e If you have 1 entry you should be calling
  beginRemoveRows(0, 0);
  
  BUG: 396175

TEST PLAN
  Code review of a common mistake
  Other usages in the same code were fine

REPOSITORY
  R159 KActivities Statistics

BRANCH
  master

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

AFFECTED FILES
  src/resultmodel.cpp

To: davidedmundson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13510: Add XDG WM Base support to our XDGShell API

2018-07-04 Thread David Edmundson
davidedmundson added a comment.


  > Or would it make sense to wait on merging until Qt 5.12 with XDG WM Base 
support is available to have more test candidates?
  
  I don't think it's needed. The protocol is 90% the same it's mostly just 
awkward name changes. Same on the Qt side.
  
  We haven't changed public API here either, other than a new enum value, so 
absolutely worst case we can just revert the change in KWin in the unlikely 
event that we see problems.

REPOSITORY
  R127 KWayland

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

To: davidedmundson, #kwin
Cc: romangg, zzag, kde-frameworks-devel, michaelh, ngraham, bruns


D13892: Restore compatibility of UDS::insert

2018-07-04 Thread David Faure
dfaure added inline comments.

INLINE COMMENTS

> udsentry.h:148
>   */
>  void insert(uint field, const QString &value);
>  

KIOCORE_DEPRECATED void...

> udsentry.h:156
>   */
>  void insert(uint field, long long l);
>  

KIOCORE_DEPRECATED void...

REPOSITORY
  R241 KIO

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

To: jtamate, dfaure, #frameworks, aacid
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13892: Restore compatibility of UDS::insert

2018-07-04 Thread Jaime Torres Amate
jtamate updated this revision to Diff 37163.
jtamate added a comment.


  Now better.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13892?vs=37161&id=37163

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

AFFECTED FILES
  src/core/udsentry.cpp
  src/core/udsentry.h

To: jtamate, dfaure, #frameworks, aacid
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13892: Restore compatibility of UDS::insert

2018-07-04 Thread Jaime Torres Amate
jtamate created this revision.
jtamate added reviewers: dfaure, Frameworks, aacid.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
jtamate requested review of this revision.

REVISION SUMMARY
  Restore KIO backwards compatibility

TEST PLAN
  kio tests passes

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/core/udsentry.cpp
  src/core/udsentry.h

To: jtamate, dfaure, #frameworks, aacid
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13885: taglibextractor: Restore extracting audio props without tags existing

2018-07-04 Thread Friedrich W. H. Kossebau
kossebau added a comment.


  In D13885#287042 , @astippich 
wrote:
  
  > I just checked all the no-meta files. The reason that they did not cause 
the tests to fail is that they still have at least one tag defined that is not 
read (encoder settings for example).
  
  
  Okay, so can confirm that what I wrote in the description/summary is correct 
;)
  
  > I think it would be better to completely remove the tags from the no-meta 
files instead of adding another test file.
  
  Fine with me. I did not spent time thinking about whether the almost 
tag-empty files are covering proper test cases or if they should have been 
really empty, as in tag-free :)
  
  > You can easily do that with the kid3 tag editor, but I can also do that if 
you prefer.
  
  I used `id3v2 -f test.mp3` to create the test.mp3 without any id tags from 
the existing :) But had to goggle up how to do that, so happy to leave this to 
people who have experience :)
  So happy to have you take over this patch, all I want is to have the tests 
fixed :)

REPOSITORY
  R286 KFileMetaData

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

To: kossebau, astippich, mgallien, michaelh
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams


D13813: Make this test work again with new uds implementation

2018-07-04 Thread David Faure
dfaure added a comment.


  In D13813#287032 , @jtamate wrote:
  
  > > But ForwardingSlaveBase::prepareUDSEntry in KIO itself, uses insert() to 
replace UDS_URL, and in fact all of ForwardingSlaveBase should use replace(), 
as well as users of ForwardingSlaveBase like kio_desktop.
  >
  > In ForwardingSlaveBase::prepareUDSEntry, it uses insert() only to insert an 
UDS URL, because it is done only if the UDS URL is empty, isn't it?
  >  But for UDS_LOCAL_PATH, I guess you're right, it should use replace().
  
  
  No, it replaces the URL if it was found, i.e. not empty.
  
  In any case, can you first restore compat?

REPOSITORY
  R318 Dolphin

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

To: jtamate, #dolphin, #frameworks, elvisangelaccio
Cc: dfaure, aacid, ngraham, bruns, elvisangelaccio, kfm-devel, spoorun, 
navarromorales, firef, andrebarros, emmanuelp


D13885: taglibextractor: Restore extracting audio props without tags existing

2018-07-04 Thread Alexander Stippich
astippich added a comment.


  Sorry for causing the regressions and thanks for the fix! 
  I just checked all the no-meta files. The reason that they did not cause the 
tests to fail is that they still have at least one tag defined that is not read 
(encoder settings for example). I think it would be better to completely remove 
the tags from the no-meta files instead of adding another test file.
  You can easily do that with the kid3 tag editor, but I can also do that if 
you prefer.

REPOSITORY
  R286 KFileMetaData

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

To: kossebau, astippich, mgallien, michaelh
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams


D13813: Make this test work again with new uds implementation

2018-07-04 Thread Jaime Torres Amate
jtamate added a comment.


  > But ForwardingSlaveBase::prepareUDSEntry in KIO itself, uses insert() to 
replace UDS_URL, and in fact all of ForwardingSlaveBase should use replace(), 
as well as users of ForwardingSlaveBase like kio_desktop.
  
  In ForwardingSlaveBase::prepareUDSEntry, it uses insert() only to insert an 
UDS URL, because it is done only if the UDS URL is empty, isn't it?
  But for UDS_LOCAL_PATH, I guess you're right, it should use replace().

REPOSITORY
  R318 Dolphin

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

To: jtamate, #dolphin, #frameworks, elvisangelaccio
Cc: dfaure, aacid, ngraham, bruns, elvisangelaccio, kfm-devel, spoorun, 
navarromorales, firef, andrebarros, emmanuelp


D13813: Make this test work again with new uds implementation

2018-07-04 Thread David Faure
dfaure added a comment.


  Albert, you are of course fully right.
  
  My reasoning was "in practice, [outside of strangely written tests such as 
the one here], there is zero reason to replace entries in a UDSEntry. All 
kioslaves either create a new UDSEntry for every file, or do reuse it after 
calling clear(). And people use if() statements rather than overwriting what 
they just wrote for the same file, which would be rather inefficient.". And 
indeed I just checked a lot of kioslaves and they do that.
  
  But ForwardingSlaveBase::prepareUDSEntry in KIO itself, uses insert() to 
replace UDS_URL, and in fact all of ForwardingSlaveBase should use replace(), 
as well as users of ForwardingSlaveBase like kio_desktop.
  That is troublesome indeed.
  
  The naming is the problem. Naming aside, the old method should still 
insert-or-replace and some new method should insert-only, but how to call that 
in a nice way other than insert, especially when 90% of the code out there 
should ONLY use that? Hmm, fastInsert?
  
  Then if we have fastInsert() and replace(), we can have a deprecated insert() 
that calls replace().

REPOSITORY
  R318 Dolphin

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

To: jtamate, #dolphin, #frameworks, elvisangelaccio
Cc: dfaure, aacid, ngraham, bruns, elvisangelaccio, kfm-devel, spoorun, 
navarromorales, firef, andrebarros, emmanuelp


D13869: [solid] Notify when interface to mounted fs is lost

2018-07-04 Thread Ben Cooksley
bcooksley added a comment.


  Predicates are Solid's query language for searching for only the devices you 
are interested in.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bcooksley, bruns, kde-frameworks-devel, michaelh, ngraham


D12820: Add KWayland virtual desktop protocol

2018-07-04 Thread Roman Gilg
romangg added a comment.


  In D12820#286961 , @mart wrote:
  
  > But.. how would you implement "move this window to a new desktop" in that 
case?
  
  
  This functionality is part of the (amended) plasma window management protocol 
and not of the new VD protocol.
  
  Btw the current diff is somewhat broken. It incorporates besides your changes 
other changes from the last few weeks.

REPOSITORY
  R127 KWayland

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

To: mart, #kwin, #plasma, graesslin, hein
Cc: davidedmundson, zzag, bshah, romangg, kde-frameworks-devel, michaelh, 
ngraham, bruns


D13888: JavaScript: Add unicode escape `\u{1234\}`

2018-07-04 Thread Nibaldo González
nibags created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
nibags requested review of this revision.

REVISION SUMMARY
  - ECMAScript 2015 Language Specification. 11.8.4 String Literals: 
https://ecma-international.org/ecma-262/6.0/#sec-literals-string-literals
  - JavaScript character escape sequences. ECMAScript 6: Unicode code point 
escapes: https://mathiasbynens.be/notes/javascript-escapes#unicode-code-point

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  fix-js-coffee

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

AFFECTED FILES
  data/syntax/javascript.xml

To: nibags
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13808: Fix KMainWindow saving incorrect widget settings

2018-07-04 Thread Mladen Milinkovic
maxrd2 added a comment.


  Relative Qt Bug is here: https://bugreports.qt.io/browse/QTBUG-69277
  Seems it's not their bug. And Qt guy commented there with:
  
  > ... You should, however, not save the window layout anymore then, because 
after closeEvent() any other widgets or child windows could also be gone 
already...

REPOSITORY
  R263 KXmlGui

BRANCH
  fix-window-state-save

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

To: maxrd2, #kde_applications, dfaure, elvisangelaccio, broulik, cfeck
Cc: wbauer, aacid, ngraham, kde-frameworks-devel, michaelh, bruns


D12820: Add KWayland virtual desktop protocol

2018-07-04 Thread Marco Martin
mart added a comment.


  In D12820#286960 , @romangg wrote:
  
  > The VD protocol was a pure "consumer" protocol before this last revision. 
Now it gets more complicated by putting requests into it for asking the 
compositor to change its configuration. I thought this was supposed to go 
through Dbus. How do you plan to restrict
  >
  >   sandboxed apps in order to not mess with the compositor configuration now?
  
  
  It would be a restricted one, usable only by plasma, like plasmashell 
protocol and such
  Tough is a good point... it may make sense to go trough dbus instead (so the 
KCM can use this as well)
  But.. how would you implement "move this window to a new desktop" in that 
case?

REPOSITORY
  R127 KWayland

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

To: mart, #kwin, #plasma, graesslin, hein
Cc: davidedmundson, zzag, bshah, romangg, kde-frameworks-devel, michaelh, 
ngraham, bruns


D12820: Add KWayland virtual desktop protocol

2018-07-04 Thread Roman Gilg
romangg added a comment.


  The VD protocol was a pure "consumer" protocol before this last revision. Now 
it gets more complicated by putting requests into it for asking the compositor 
to change its configuration. I thought this was supposed to go through Dbus. 
How do you plan to restrict 
   sandboxed apps in order to not mess with the compositor configuration now?

REPOSITORY
  R127 KWayland

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

To: mart, #kwin, #plasma, graesslin, hein
Cc: davidedmundson, zzag, bshah, romangg, kde-frameworks-devel, michaelh, 
ngraham, bruns


D13880: [KMoreTools] Reduce menu hierarchy

2018-07-04 Thread Nathaniel Graham
ngraham added a comment.


  If it's important for uninstalled tools to be not always be visible, maybe 
what we should do is embed the //content// of the KNewStuff menu in Spectacle 
rather than exposing the whole thing as a sub-menu. Then we could make a few 
string changes, and it would be like this in Spectacle:
  
  No apps installed:
  
Open Screenshots folder
Print...
Install apps to Record screen>
  
  One app installed:
  
Open Screenshots folder
Print...
--
 Record screen
Open SimpleScreenRecorder
Install alternatives >   Peek
 Vokoscreen
  
  All apps installed:
  
Open Screenshots folder
Print...
--
 Record screen
Open SimpleScreenRecorder
Open Peek
Open Vokoscreen
  
  This would have the benefit that you could avoid a level of hierarchy for the 
workflow of actually actually opening an installed screen recording app in 
Spectacle, but would have the disadvantage that it would involve some 
additional code changes in Spectacle (and other apps that include the KNewStuff 
menu as a sub-menu).

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13869: [solid] Notify when interface to mounted fs is lost

2018-07-04 Thread Anthony Fieroni
anthonyfieroni added a comment.


  I'm aware that does not an idea, what is the purpose of Predicate. What 
should i search in Solid::Predicate::matches?

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham


D12820: Add KWayland virtual desktop protocol

2018-07-04 Thread Marco Martin
mart updated this revision to Diff 37149.
mart added a comment.


  - add a request remove to the protocol
  - add api call to request removal of a desktop
  - requestCreate
  - remove window management features:
  - active() -> isactive()
  - requestEnterNewVirtualDesktop

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12820?vs=36888&id=37149

BRANCH
  mart/plasmavirtualdesktop

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

AFFECTED FILES
  .arclint
  CMakeLists.txt
  autotests/client/CMakeLists.txt
  autotests/client/test_datadevice.cpp
  autotests/client/test_plasma_virtual_desktop.cpp
  autotests/client/test_plasma_window_model.cpp
  autotests/client/test_plasmashell.cpp
  autotests/client/test_wayland_registry.cpp
  autotests/client/test_wayland_surface.cpp
  autotests/client/test_wayland_windowmanagement.cpp
  autotests/client/test_xdg_output.cpp
  autotests/client/test_xdg_shell_v6.cpp
  src/client/CMakeLists.txt
  src/client/plasmashell.cpp
  src/client/plasmashell.h
  src/client/plasmavirtualdesktop.cpp
  src/client/plasmavirtualdesktop.h
  src/client/plasmawindowmanagement.cpp
  src/client/plasmawindowmanagement.h
  src/client/plasmawindowmodel.cpp
  src/client/plasmawindowmodel.h
  src/client/protocols/fullscreen-shell.xml
  src/client/protocols/outputdevice.xml
  src/client/protocols/plasma-shell.xml
  src/client/protocols/plasma-virtual-desktop.xml
  src/client/protocols/plasma-window-management.xml
  src/client/protocols/xdg-output-unstable-v1.xml
  src/client/registry.cpp
  src/client/registry.h
  src/client/textinput.h
  src/client/textinput_v0.cpp
  src/client/textinput_v2.cpp
  src/client/xdgoutput.cpp
  src/client/xdgoutput.h
  src/client/xdgshell_v6.cpp
  src/server/CMakeLists.txt
  src/server/datadevice_interface.cpp
  src/server/display.cpp
  src/server/display.h
  src/server/plasmashell_interface.cpp
  src/server/plasmashell_interface.h
  src/server/plasmavirtualdesktop_interface.cpp
  src/server/plasmavirtualdesktop_interface.h
  src/server/plasmawindowmanagement_interface.cpp
  src/server/plasmawindowmanagement_interface.h
  src/server/surface_interface.cpp
  src/server/xdgforeign_v2_interface.cpp
  src/server/xdgoutput_interface.cpp
  src/server/xdgoutput_interface.h
  src/tools/mapping.txt
  tests/plasmasurfacetest.cpp

To: mart, #kwin, #plasma, graesslin, hein
Cc: davidedmundson, zzag, bshah, romangg, kde-frameworks-devel, michaelh, 
ngraham, bruns


D13885: taglibextractor: Restore extracting audio props without tags existing

2018-07-04 Thread Friedrich W. H. Kossebau
kossebau created this revision.
kossebau added reviewers: astippich, mgallien, michaelh.
Restricted Application added projects: Frameworks, Baloo.
Restricted Application added subscribers: Baloo, kde-frameworks-devel.
kossebau requested review of this revision.

REVISION SUMMARY
  7f9de32eedff7f81818145001fc38bbed01de1b7 
 
made extraction of audio
  properties depending on the presence of any tag, by moving the extraction
  into the "if (!tags->isEmpty())" branch.
  This dependency is not necessary and prevents extracting the audio data
  for files without a tag (as sideeffect broke a test in baloo-widgets).
  
  Existing sample autotest files in no-meta/ subdir have not catched this,
  as they still had an ID3 tag content (TSSE), so tags->isEmpty() was false.
  
  Patch moves the existing files in a new subdir empty-meta/, to reflect they
  have some metadata but appear empty for the extraction. And adds a new test
  file with no metadata at all (for now only mp3 as I have no idea about
  metadata for the others formats)

TEST PLAN
  New no-meta/test.mp3 sample file no longer fails with the test.
  Unit test extractortest in baloo-widgets also works again, as audio metadata
  is extracted again from a file with no id3 data.

REPOSITORY
  R286 KFileMetaData

BRANCH
  restoreaudiopropextractionwithouttags

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

AFFECTED FILES
  autotests/samplefiles/empty-meta/test.flac
  autotests/samplefiles/empty-meta/test.m4a
  autotests/samplefiles/empty-meta/test.mp3
  autotests/samplefiles/empty-meta/test.mpc
  autotests/samplefiles/empty-meta/test.ogg
  autotests/samplefiles/empty-meta/test.opus
  autotests/samplefiles/no-meta/test.flac
  autotests/samplefiles/no-meta/test.m4a
  autotests/samplefiles/no-meta/test.mp3
  autotests/samplefiles/no-meta/test.mpc
  autotests/samplefiles/no-meta/test.ogg
  autotests/samplefiles/no-meta/test.opus
  autotests/taglibextractortest.cpp
  src/extractors/taglibextractor.cpp

To: kossebau, astippich, mgallien, michaelh
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams


D13880: [KMoreTools] Reduce menu hierarchy

2018-07-04 Thread Nicolas Fella
nicolasfella added a comment.


  In D13880#286816 , @ngraham wrote:
  
  > Or even this:
  >
  >   Open SimpleScreenRecorder
  >   -
  >   Install Peek
  >   Install Vokoscreen
  >   -
  >   Configure...
  >
  
  
  Put into code it would look like
  
  F6013983: Screenshot_20180704_172717.png 

  
  Pro: Very flat hierarchy, code could be simplified a lot
  Con: Unwanted and uninstalled tools would be present all the time.
  
  Before I proceed with any idea I would like to have an opinion from @gregormi 
or @dhaumann

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13869: [solid] Notify when interface to mounted fs is lost

2018-07-04 Thread Stefan Brüns
bruns added a comment.


  In D13869#286746 , @broulik wrote:
  
  > Sure, but the device isn't "removed", so emitting a remove signal for 
something that is still there, is wrong, isn't it? Perhaps we could signal a 
removal if the device lost *all* its interfaces? But it probably doesn't in the 
unmount case.
  
  
  a valid device/dbus object has at least the `org.freedesktop.DBus.Properties` 
interface, so this signal is already emitted when the device is removed (== 
last interface removed).
  
  I think the code needs some overhauling - AFAIK it was written when UDisks2 
had a much more simplistic interface. The whole properties cache is quite 
broken ("something has changed - throw away the cache - fetch everything again" 
, "we don't know the property - lets fetch it again"), we are doing to many 
roundtrips.
  
  The list of properties is announced in the DBus introspection data for each 
interface. There is no need for the "negative" cache. We are mixing properties 
from different interfaces (e.g. size, a filesystem size may differ from the 
partition size).
  
  While the intention of the patch is right, I don't think its the right way to 
do it. I would favor a solution which signals changes in the `Predicate` 
matches. This is definitely more work, but otherwise we are papering over one 
issue after the other.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham


D13403: Android: Make it possible to override a target's APK directory

2018-07-04 Thread Aleix Pol Gonzalez
This revision was automatically updated to reflect the committed changes.
Closed by commit R240:d6cc583f32a4: Android: Make it possible to override a 
target's APK directory (authored by apol).

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13403?vs=37024&id=37147

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

AFFECTED FILES
  toolchain/Android.cmake
  toolchain/ECMAndroidDeployQt.cmake
  toolchain/deployment-file.json.in

To: apol, #frameworks, vkrause
Cc: vkrause, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns


D13884: [KMessageWidget] Update stylesheet when palette changes

2018-07-04 Thread Nathaniel Graham
ngraham accepted this revision.
ngraham added a comment.


  Thanks, works as expected and I couldn't find any regressions. Code looks 
good.

REPOSITORY
  R236 KWidgetsAddons

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

To: broulik, #frameworks, rjvbb, ngraham, cfeck
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13884: [KMessageWidget] Update stylesheet when palette changes

2018-07-04 Thread René J . V . Bertin
rjvbb accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R236 KWidgetsAddons

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

To: broulik, #frameworks, rjvbb, ngraham, cfeck
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13884: [KMessageWidget] Update stylesheet when palette changes

2018-07-04 Thread René J . V . Bertin
rjvbb added a comment.


  Otherwise LGTM (I also checked the in-app palette change myself: seems to 
work fine).

INLINE COMMENTS

> kmessagewidget.cpp:57
>  void createLayout();
> +void applyStyleSheet();
>  void updateSnapShot();

Nitpick: shouldn't this be called `setStyleSheet` or something similar that 
indicates a bit better that it defines the stylesheet itself?

REPOSITORY
  R236 KWidgetsAddons

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

To: broulik, #frameworks, rjvbb, ngraham, cfeck
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13884: [KMessageWidget] Update stylesheet when palette changes

2018-07-04 Thread Kai Uwe Broulik
broulik updated this revision to Diff 37144.
broulik added a comment.


  - Juggle it around a bit

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13884?vs=37143&id=37144

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

AFFECTED FILES
  src/kmessagewidget.cpp

To: broulik, #frameworks, rjvbb, ngraham, cfeck
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13884: [KMessageWidget] Update stylesheet when palette changes

2018-07-04 Thread Kai Uwe Broulik
broulik edited the test plan for this revision.

REPOSITORY
  R236 KWidgetsAddons

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

To: broulik, #frameworks, rjvbb, ngraham, cfeck
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13884: [KMessageWidget] Update stylesheet when palette changes

2018-07-04 Thread Kai Uwe Broulik
broulik edited the summary of this revision.

REPOSITORY
  R236 KWidgetsAddons

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

To: broulik, #frameworks, rjvbb, ngraham, cfeck
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13884: [KMessageWidget] Update stylesheet when palette changes

2018-07-04 Thread Kai Uwe Broulik
broulik created this revision.
broulik added reviewers: Frameworks, rjvbb, ngraham, cfeck.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
broulik requested review of this revision.

REVISION SUMMARY
  The colors are hardcoded right now but it's still a bug that the widget 
doesn't react to `PaletteChange` in any way

TEST PLAN
  Changed system colors, `applyStylesheet` was called. Initially also looks fine

REPOSITORY
  R236 KWidgetsAddons

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

AFFECTED FILES
  src/kmessagewidget.cpp

To: broulik, #frameworks, rjvbb, ngraham, cfeck
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13880: [KMoreTools] Reduce menu hierarchy

2018-07-04 Thread Nicolas Fella
nicolasfella added a comment.


  In D13880#286820 , @ngraham wrote:
  
  > In D13880#286818 , @nicolasfella 
wrote:
  >
  > > Do you know if/how we can check if there is a handler for appstream://? 
So we can fall back to website if none is available
  >
  >
  > I think you added that data and functionality in D13706 
, no? Looks like you can just try to get 
`appstreamId`, and if you get back an empty string, then no appstream URL is 
defined for it and you can fall back to the homepage.
  
  
  No, what I mean is check if Discover or any app that handles appstream:// is 
available. The install action is pretty useless without an application handling 
it.

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13880: [KMoreTools] Reduce menu hierarchy

2018-07-04 Thread Nathaniel Graham
ngraham added a comment.


  In D13880#286818 , @nicolasfella 
wrote:
  
  > Do you know if/how we can check if there is a handler for appstream://? So 
we can fall back to website if none is available
  
  
  I think you added that data and functionality in D13706 
, no? Looks like you can just try to get 
`appstreamId`, and if you get back an empty string, then no appstream URL is 
defined for it and you can fall back to the homepage.

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13880: [KMoreTools] Reduce menu hierarchy

2018-07-04 Thread Nicolas Fella
nicolasfella added a comment.


  In D13880#286800 , @ngraham wrote:
  
  > You are the most awesome person in the world today.
  
  
  Yay!
  
  > Here's a radical thought: get rid of the More menu entirely and move all of 
its content inline, like so:
  
  Sound good to me
  
  > And clicking on the menu items for any of the Install options would open 
the appstream:// URL, falling back to the home page if no AppStream information 
is available. I think that would be super slick.
  
  That would have been my next patch. Do you know if/how we can check if there 
is a handler for appstream://? So we can fall back to website if none is 
available

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13880: [KMoreTools] Reduce menu hierarchy

2018-07-04 Thread Nathaniel Graham
ngraham added a comment.


  Or even this:
  
Open SimpleScreenRecorder
-
Install Peek
Install Vokoscreen
-
Configure...
  
  ...And clicking on the menu items for any of the Install options would open 
the `appstream://` URL, falling back to the home page if no AppStream 
information is available. I think that would be super slick.

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13880: [KMoreTools] Reduce menu hierarchy

2018-07-04 Thread Nathaniel Graham
ngraham added a comment.


  You are the most awesome person in the world today.
  
  Here's a radical thought: get rid of the More menu entirely and move all of 
its content inline, like so:
  
  Installed:
SimpleScreenRecorder
-
Not installed:
Peek   >
Vokoscreen >
-
Configure...

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13880: [KMoreTools] Reduce menu hierarchy

2018-07-04 Thread Nicolas Fella
nicolasfella edited the test plan for this revision.

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13880: [KMoreTools] Reduce menu hierarchy

2018-07-04 Thread Nicolas Fella
nicolasfella created this revision.
nicolasfella added reviewers: gregormi, dhaumann, ngraham.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
nicolasfella requested review of this revision.

REVISION SUMMARY
  Skip 'More' submenu and attach its children directly to the menu when 'More' 
is the only submenu.

REPOSITORY
  R304 KNewStuff

BRANCH
  hidemore

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

AFFECTED FILES
  src/kmoretools/kmoretools.cpp
  src/kmoretools/kmoretools.h

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13386: haskell.xml: remove types from "prelude function" section

2018-07-04 Thread Li-yao Xia
xialiyao added a reviewer: Framework: Syntax Highlighting.

REPOSITORY
  R216 Syntax Highlighting

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

To: xialiyao, #framework_syntax_highlighting
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13373: haskell.xml: highlight promoted data constructors

2018-07-04 Thread Li-yao Xia
xialiyao added a reviewer: Framework: Syntax Highlighting.

REPOSITORY
  R216 Syntax Highlighting

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

To: xialiyao, #framework_syntax_highlighting
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


D13370: haskell.xml: add keywords family, forall, pattern

2018-07-04 Thread Li-yao Xia
xialiyao added a reviewer: Framework: Syntax Highlighting.

REPOSITORY
  R216 Syntax Highlighting

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

To: xialiyao, #framework_syntax_highlighting
Cc: kde-frameworks-devel, michaelh, ngraham, bruns


KDE CI: Frameworks kwayland kf5-qt5 FreeBSDQt5.10 - Build # 25 - Still Unstable!

2018-07-04 Thread CI System
BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kwayland%20kf5-qt5%20FreeBSDQt5.10/25/
 Project:
Frameworks kwayland kf5-qt5 FreeBSDQt5.10
 Date of build:
Wed, 04 Jul 2018 10:30:42 +
 Build duration:
36 min and counting
   JUnit Tests
  Name: (root) Failed: 14 test(s), Passed: 25 test(s), Skipped: 0 test(s), Total: 39 test(s)Failed: TestSuite.kwayland-testCompositorFailed: TestSuite.kwayland-testDataDeviceFailed: TestSuite.kwayland-testDataSourceFailed: TestSuite.kwayland-testRegionFailed: TestSuite.kwayland-testRemoteAccessFailed: TestSuite.kwayland-testSelectionFailed: TestSuite.kwayland-testShmPoolFailed: TestSuite.kwayland-testSubCompositorFailed: TestSuite.kwayland-testSubSurfaceFailed: TestSuite.kwayland-testWaylandConnectionThreadFailed: TestSuite.kwayland-testWaylandRegistryFailed: TestSuite.kwayland-testWaylandServerDisplayFailed: TestSuite.kwayland-testWaylandShellFailed: TestSuite.kwayland-testWaylandSurface

KDE CI: Frameworks kwayland kf5-qt5 SUSEQt5.9 - Build # 35 - Still Unstable!

2018-07-04 Thread CI System
BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kwayland%20kf5-qt5%20SUSEQt5.9/35/
 Project:
Frameworks kwayland kf5-qt5 SUSEQt5.9
 Date of build:
Wed, 04 Jul 2018 10:30:42 +
 Build duration:
9 min 30 sec and counting
   JUnit Tests
  Name: (root) Failed: 3 test(s), Passed: 41 test(s), Skipped: 0 test(s), Total: 44 test(s)Failed: TestSuite.kwayland-testRemoteAccessFailed: TestSuite.kwayland-testSelectionFailed: TestSuite.kwayland-testWaylandSeat
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report57%
(4/7)90%
(222/248)90%
(222/248)85%
(22990/26939)52%
(9124/17520)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests.client98%
(39/40)98%
(39/40)96%
(10972/11389)49%
(5844/12023)autotests.server100%
(5/5)100%
(5/5)99%
(353/356)49%
(169/344)src.client97%
(69/71)97%
(69/71)84%
(5341/6360)66%
(1323/2012)src.server96%
(109/113)96%
(109/113)86%
(6324/7340)66%
(1788/2728)src.tools0%
(0/2)0%
(0/2)0%
(0/693)0%
(0/272)src.tools.testserver0%
(0/3)0%
(0/3)0%
(0/69)0%
(0/10)tests0%
(0/14)0%
(0/14)0%
(0/732)0%
(0/131)

KDE CI: Frameworks kwayland kf5-qt5 SUSEQt5.10 - Build # 63 - Still Unstable!

2018-07-04 Thread CI System
BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kwayland%20kf5-qt5%20SUSEQt5.10/63/
 Project:
Frameworks kwayland kf5-qt5 SUSEQt5.10
 Date of build:
Wed, 04 Jul 2018 10:30:42 +
 Build duration:
9 min 44 sec and counting
   JUnit Tests
  Name: (root) Failed: 4 test(s), Passed: 40 test(s), Skipped: 0 test(s), Total: 44 test(s)Failed: TestSuite.kwayland-testRemoteAccessFailed: TestSuite.kwayland-testSelectionFailed: TestSuite.kwayland-testWaylandSeatFailed: TestSuite.kwayland-testWaylandShell
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report57%
(4/7)89%
(221/248)89%
(221/248)82%
(22193/26918)50%
(8691/17520)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests.client95%
(38/40)95%
(38/40)91%
(10388/11384)46%
(5528/12023)autotests.server100%
(5/5)100%
(5/5)99%
(353/356)49%
(169/344)src.client97%
(69/71)97%
(69/71)83%
(5258/6350)62%
(1244/2012)src.server96%
(109/113)96%
(109/113)84%
(6194/7334)64%
(1750/2728)src.tools0%
(0/2)0%
(0/2)0%
(0/693)0%
(0/272)src.tools.testserver0%
(0/3)0%
(0/3)0%
(0/69)0%
(0/10)tests0%
(0/14)0%
(0/14)0%
(0/732)0%
(0/131)

D13559: Fix some of cppcheck warnings

2018-07-04 Thread Bhushan Shah
This revision was automatically updated to reflect the committed changes.
Closed by commit R127:84bc3cbd6102: Fix some of cppcheck warnings (authored by 
bshah).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13559?vs=37088&id=37135

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

AFFECTED FILES
  autotests/server/test_display.cpp
  src/client/xdgoutput.cpp
  src/server/dpms_interface.cpp
  src/server/xdgoutput_interface.cpp

To: bshah, #frameworks, aacid
Cc: apol, aacid, kde-frameworks-devel, michaelh, ngraham, bruns


D13627: [KIconThemes] Isolate private data from race conditions

2018-07-04 Thread Anthony Fieroni
anthonyfieroni added a reviewer: hein.
anthonyfieroni added a subscriber: hein.
anthonyfieroni added a comment.


  It happen again, so it's not a threading issue
  @hein when you hover grouped task in taskmanager end up with broken icons, 
sometimes, kcache in ~/.cache is generated at same that icons are broken in 
plasmashell
  
  1. single thread wrong algorithm generating / read cache
  2. other process (kbuildsyscoca?!) corrupt cache while kiconloader read it
  
  what you think?

REPOSITORY
  R302 KIconThemes

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

To: anthonyfieroni, davidedmundson, dfaure, #frameworks, hein
Cc: hein, kde-frameworks-devel, michaelh, ngraham, bruns


D13510: Add XDG WM Base support to our XDGShell API

2018-07-04 Thread Roman Gilg
romangg added a comment.


  From my side I can't see anything wrong right now. Since we would merge after 
the next release anyways we have some time to correct any overlooked mistakes 
afterwards. Or would it make sense to wait on merging until Qt 5.12 with XDG WM 
Base support is available to have more test candidates?

INLINE COMMENTS

> xdgshell_stable.cpp:2
> +/
> +Copyright 2017  David Edmundson 
> +

2018 - other files below as well.

REPOSITORY
  R127 KWayland

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

To: davidedmundson, #kwin
Cc: romangg, zzag, kde-frameworks-devel, michaelh, ngraham, bruns


D13869: [solid] Notify when interface to mounted fs is lost

2018-07-04 Thread Anthony Fieroni
anthonyfieroni added a comment.


  So about docs when interface is added it will notify again for mounting, i 
think that's the idea behind interfaces.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham


D13869: [solid] Notify when interface to mounted fs is lost

2018-07-04 Thread Kai Uwe Broulik
broulik added a comment.


  Sure, but the device isn't "removed", so emitting a remove signal for 
something that is still there, is wrong, isn't it? Perhaps we could signal a 
removal if the device lost *all* its interfaces? But it probably doesn't in the 
unmount case.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham


D13869: [solid] Notify when interface to mounted fs is lost

2018-07-04 Thread Anthony Fieroni
anthonyfieroni added a comment.


  So if KFilePlacesModel wants to hide, i don't see how this will break things, 
it only notify when udisk2 loose interface to mounted fs.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham


D13816: Make KItinerary work as a static library

2018-07-04 Thread Volker Krause
vkrause added a comment.


  In D13816#286689 , @apol wrote:
  
  > It does have some advantages, I personally thing though that we shouldn't 
be compromising code readability and convenience for size, but I don't know how 
much better it is shared vs static.
  
  
  Yep. I don't have numbers yet for KDE examples (as we can't build a working 
test-case at this point), from what I have seen at work I'd expect something in 
the 2x-3x range compared to a dynamic build, as well as some small startup 
speed gains (in the 100ms range).
  Maybe it's just me, but our APKs still feel a bit heavy to me (KDE Itinerary 
is now at 27M here, for what's essentially a list view), therefore I'm 
exploring this option a bit.
  
  > If we need to have a list of `::init()` with all frameworks 
we depend on, it will be a bit odd and error prone, but it could be useful.
  
  Yep, I don't like this at all either. However, you will only need this in 
your application code, and only if you explicitly want to statically link it 
(ie. no impact for "normal" applications). And it wont be needed for all libs 
either, only those that rely on static construction somehow (qrc, qm catalog 
loaders, etc) and don't have a natural entry point or (internal) singleton 
already anyway (my best guess at this point, maybe 1/3). Not that this makes it 
less error prone though, on the contrary even.
  
  So for me this boils down to: do we want to enable users of (some of) our 
frameworks to do static builds and are we willing to accept the ugliness this 
brings with it?
  
  Or does anyone have an idea how to solve this more elegantly? :)

REPOSITORY
  R1003 KItinerary: Travel Reservation handling library

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

To: vkrause, #frameworks
Cc: apol, kde-pim, dvasin, rodsevich, winterz, vkrause, mlaurent, knauss, 
dvratil