D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau updated this revision to Diff 32916.
kossebau added a comment.


  Updating with proposed further massaging of the API
  
  - changed some class/method names to closely follow KRunner terms: 
(RunnerContext, query)
  - moved RunnerContext into separate header (for one per class)
  - moving submit/cancel methods back to AbstractRunner
  
  Keeping RunnerContext a pure data container and AbstractRunner as responsible
  for creating and consuming these objects feels more balanced and means less
  spreading of responsibilities/functionality.
  
  The auto-submit in the destructor of RunnerContext left a feel of lack of
  control to the API user given the passing of RunnerContext as sharedpointer.
  Also reading the code and seeing only startMatching, one would wonder how
  actually the matching is completed. Having to read up in the API dox might
  not make up for the otherwise unneeded additional call.
  
  One challenge I found when tinkering around with this:
  a D-Bus runner could in theory receive multiple calls at the same time, from
  one or multiple processes. While the RunnerContext now would allow handling
  multiple overlapping requests easily, I have yet to get an idea how/if
  overlapping D-Bus calls work in general, with QtDBus in detail and even more
  how the D-Bus proxy krunner acts when a newer query is asked for (e.g. on
  quick typing). The current code at least should be prepared in theory to
  some bit.

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10078?vs=31871=32916

BRANCH
  kdbusrunnerlib2

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

AFFECTED FILES
  CMakeLists.txt
  KF5DBusRunnerConfig.cmake.in
  autotests/CMakeLists.txt
  autotests/testremoterunner.cpp
  autotests/testremoterunner.h
  src/CMakeLists.txt
  src/dbusrunner/CMakeLists.txt
  src/dbusrunner/abstractrunner.cpp
  src/dbusrunner/abstractrunner.h
  src/dbusrunner/abstractrunner_p.cpp
  src/dbusrunner/abstractrunner_p.h
  src/dbusrunner/action.h
  src/dbusrunner/dbusadaptor.cpp
  src/dbusrunner/dbusadaptor_p.h
  src/dbusrunner/querymatch.h
  src/dbusrunner/runnercontext.cpp
  src/dbusrunner/runnercontext.h
  src/dbusrunner/runnercontext_p.h
  src/querymatch.h

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau commandeered this revision.
kossebau edited reviewers, added: davidedmundson; removed: kossebau.

REPOSITORY
  R308 KRunner

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

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau added a comment.


  In D10078#252871 , @davidedmundson 
wrote:
  
  > This makes it quite easy for a developer to screw up and block krunner.
  >  The RAII approach makes it very very hard for a developer to screw up with 
any of the multiple usage patterns.
  
  
  Seems we have two different main design motives then: mine was to keep the 
classes/concepts close to the in-process krunner plugin ones, yours is to try 
to lower the chance of blocked calls :)
  
  Guess I am not so burned by blocked krunner plugins to give that so much 
weight, as I also do not see the danger of people forgetting to do any 
finishMatching call. And would argue that the D-Bus Krunner.side proxy should 
protect itself in general against remote D-Bus krunner services locking 
themselves up in other ways.
  
  Your project, your call/decision :)
  
  I propose two renames though:
  
  1. class "*Context" -> "MatchReply" (or similar/better): the "*Context" class 
in normal in-process krunner plugins has a different, passive purpose, also 
might "submit/cancel a context" not make that much sense? (non-native speaker 
here, might miss out phrase meanings)
  2. request handling method name "startMatching" -> "handleMatchRequest"(or 
similar/better): for a method "start*" I would expect some counterpart methods 
on the same class/object.
  
  I could not remember a Qt/KF class following the same pattern (something like 
single-thread service class with a handler method to do async request replies), 
so no existing naming pattern handy for proposal.
  
  At least I imagine that as developer having to reimplement
  
virtual void handleMatchRequest(const MatchReply::Ptr );
  
  one has a better idea about what to do, compared to
  
virtual void startMatching(const RunnerContext::Ptr );
  
  Even if the mixing of request properties (query string) into the reply class 
spoils the concepts a little. But splitting that off into another separate 
object makes the code more complex again, so that trade-off could be fine.

REPOSITORY
  R308 KRunner

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

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D12355: [API dox] New UI marker @info:placeholder

[Friedrich W . H . Kossebau]

kossebau updated this revision to Diff 32604.
kossebau added a comment.


  add also in kuit handling code

REPOSITORY
  R249 KI18n

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12355?vs=32572=32604

BRANCH
  addinfoplaceholderuimarker

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

AFFECTED FILES
  docs/programmers-guide.md
  src/kuitmarkup.cpp

To: kossebau, #localization, ilic
Cc: #frameworks, michaelh, bruns

D12353: [API dox] New UI marker @item:valuesuffix

[Friedrich W . H . Kossebau]

kossebau updated this revision to Diff 32605.
kossebau added a comment.


  add also in kuit handling code

REPOSITORY
  R249 KI18n

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12353?vs=32571=32605

BRANCH
  additemvaluesuffixuimaker

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

AFFECTED FILES
  docs/programmers-guide.md
  src/kuitmarkup.cpp

To: kossebau, #localization, ilic
Cc: aacid, #frameworks, michaelh, bruns

D12353: [API dox] New UI marker @item:valuesuffix

[Friedrich W . H . Kossebau]

kossebau added a comment.


  In D12353#249996 , @aacid wrote:
  
  > All the other elements of that list have an entry like
  >
  > ./src/kuitmarkup.cpp:281:SET_CUE(InrangeCue, QStringLiteral("inrange"));
  >
  > Do we need that too for valuesuffix?
  
  
  Good find, did not know there is a complete data model in code for them here. 
At least for consistency I would expect we should add this here as well, will 
update patches.

REPOSITORY
  R249 KI18n

BRANCH
  additemvaluesuffixuimaker

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

To: kossebau, #localization, ilic
Cc: aacid, #frameworks, michaelh, bruns

D11675: use KDE_INSTALL_DATADIR instead of FULL_DATADIR

[Friedrich W . H . Kossebau]

This revision was automatically updated to reflect the committed changes.
Closed by commit R290:ee1ea06d964c: use KDE_INSTALL_DATADIR instead of 
FULL_DATADIR (authored by bshah, committed by kossebau).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D11675?vs=30487=32610#toc

REPOSITORY
  R290 KPackage

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11675?vs=30487=32610

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

AFFECTED FILES
  KF5PackageMacros.cmake

To: bshah, kossebau
Cc: #frameworks, michaelh, bruns

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau added a comment.


  Good, seems we found agreed-on codebase :) Will brush over the API dox/code 
comments some more for a final thumb-up.
  
  Though after yesterday's prototyping of further dbusrunner plugins there is 
another thing where I might want to suggest some API change to be more 
future-proof (for some possible org.kde.krunner2):
  
  Currently the match requests have no support for being cancelled (not part of 
org.kde.krunner1 D-Bus API, and have not found something on the D-Bus layer how 
to signal the discarding the wait for a reply). The baloo dbusrunner simply 
only handles the latest request. which surely covers the typical use-case, 
still, it will continue to handle the latest request even if there is no longer 
interest in it. That approach might be an issue once there are more 
cpu-intensive dbusrunners, wasting resources (cpu/io) when not needed.
  
  To prepare for that it might make sense to turn `MatchReply` into a QObject, 
so it could get some signal `validChanged` or similar added once the backend 
supports discarding. While there currently already is a method to query the 
valid state, this is more a small convenience method given that right now it is 
under full control of the plugin developer when a MatchReply will get invalid, 
it will only happen on actions done via the API. But once the valid state can 
change by remote activitiy, a signal might be nice to have.
  Using a QObject would also allow to switch from the fragile 
`QVector mActiveMatchReplies` to using a QPointer-based 
approach on the real MatchReply objects, which might be less fragile.
  
  What do you think? Would give this a separate try tonight, to get some idea.

REPOSITORY
  R308 KRunner

BRANCH
  kdbusrunnerlib2

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

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D11716: Fix build on FreeBSD broken by d7cce9937d5e9af2753fadb82d11f308b58bb8fa

[Friedrich W . H . Kossebau]

kossebau added a comment.


  Thanks for coming up so quick with a fix :)
  
  You surely wanted to also wrap the include of the "sys/xattr.h" header in the 
new condition? Missing from the patch.

INLINE COMMENTS

> config-kioslave-file.h.cmake:15
> +#cmakedefine01 HAVE_SYS_XATTR_H
> \ No newline at end of file


Please add a trailing newline to the end of the file.

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, kossebau, dfaure
Cc: #frameworks, michaelh, ngraham

D11716: Fix build on FreeBSD broken by d7cce9937d5e9af2753fadb82d11f308b58bb8fa

[Friedrich W . H . Kossebau]

kossebau added a comment.


  From quick pure code reading this looks fine to me. +1
  
  Have not tested though right now, so have to leave the ship-it to actual 
Dolphin/KIO maintainers.

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, kossebau, dfaure
Cc: #frameworks, michaelh, ngraham

D11716: Fix build on FreeBSD broken by d7cce9937d5e9af2753fadb82d11f308b58bb8fa

[Friedrich W . H . Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> config-kioslave-file.h.cmake:14
> +/* Defined if system has extended file attributes support. */
> +#cmakedefine01 HAVE_SYS_XATTR_H

While FindACL.cmake sets internally the cmake variable HAVE_SYS_XATTR_H, it is 
not part of the officially set variables (cmp. notes in header of that file)

So more future-proof will be to do an explicit

  check_include_files(sys/xattr.h HAVE_SYS_XATTR_H)

in src/ioslaves/file/CMakeLists.txt instead of reyling on an undocumented 
side-effect of `find_package(ACL)`.

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, kossebau, dfaure
Cc: #frameworks, michaelh, ngraham

D11204: Support NTFS hidden files

[Friedrich W . H . Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> file_unix.cpp:40
>  #include 
> +#include 
>  #include 

This unconditional include sadly breaks the build at least on FreeBSD (see 
https://build.kde.org/view/Frameworks/job/Frameworks%20kio%20kf5-qt5%20FreeBSDQt5.9/164/console
 ) and other systems where this header is not part of the libc (?) headers.

So this needs to get some condition checking before using it. Compare other 
code doing that:
https://lxr.kde.org/search?_filestring=&_string=xattr.h&_casesensitive=1

While the xattr.h is already checked for in KIO in the FindACL.cmake file, an 
explicite check like

  check_include_files(sys/xattr.h HAVE_SYS_XATTR_H)

and passing HAVE_SYS_XATTR_H via config-kioslave-file.h.cmake into 
file_unix.cpp and checking the condition additionally to or instead of 
Q_OS_LINUX for the new code should solve this issue properly.

REPOSITORY
  R241 KIO

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

To: rominf, #dolphin, #frameworks, dfaure
Cc: kossebau, dfaure, markg, elvisangelaccio, ltoscano, anthonyfieroni, 
broulik, #frameworks, #dolphin, michaelh, ngraham

D11642: Fix the rcc binary package generation

[Friedrich W . H . Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> KF5PackageMacros.cmake:164
> +  DEPENDS ${GENERATED_RCC_CONTENTS} 
> ${component}-${root}-metadata-json ${kpkgqrc})
> +install(FILES ${GENERATED_RCC_CONTENTS} DESTINATION 
> ${KDE_INSTALL_FULL_DATADIR}/${install_dir}/${root}/${component}/ RENAME 
> contents.rcc)
>  

Does a plain `${KDE_INSTALL_DATADIR}` not also work? The non-FULL variable 
variants are the ones used usually with `install()`. Not exactly sure why they 
are, but doing here as well would be consistent at least and less surprising.

REPOSITORY
  R290 KPackage

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

To: bshah, mart, davidedmundson, apol
Cc: kossebau, #frameworks, michaelh, ngraham

D11675: use KDE_INSTALL_DATADIR instead of FULL_DATADIR

[Friedrich W . H . Kossebau]

kossebau added a comment.


  While myself I have never made use of it, the idea of using a relative path & 
not an absolute path with the DESTINATION argument is to allow overriding the 
install prefix at install time. Or in words of 
https://cmake.org/cmake/help/v3.0/command/install.html sa
  
  >   If a relative path is given it is interpreted relative to the value of 
the CMAKE_INSTALL_PREFIX variable. The prefix can be relocated at install time 
using the DESTDIR mechanism explained in the CMAKE_INSTALL_PREFIX variable 
documentation.
  
  So given all the other install macros I have seen allow to do that, let's be 
consistent here and use the relative path as well. I am not aware of any need 
to use the absolute path here directly.
  
  Not tested though due to broken setup currently, so just +1 and no shipit :)

REPOSITORY
  R290 KPackage

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

To: bshah, kossebau
Cc: #frameworks, michaelh, ngraham

D11414: [xcb] Fix implementation of _NET_WM_FULLSCREEN_MONITORS

[Friedrich W . H . Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> netwm.cpp:2845-2847
> +const uint32_t data[5] = {
> +topology.top, topology.bottom, topology.left, topology.right, 1
> +};

Seems clang (at least as of FreeBSD with -Wc++11-narrowing) does not like this 
narrowing from the int topology properties to the uint32_t array one:

  00:37:23 /usr/home/jenkins/workspace/Dependency Build Frameworks kf5-qt5 
FreeBSDQt5.9/kwindowsystem/src/platforms/xcb/netwm.cpp:2846:13: error: 
non-constant-expression cannot be narrowed from type 'int' to 'uint32_t' (aka 
'unsigned int') in initializer list [-Wc++11-narrowing]
  00:37:23 topology.top, topology.bottom, topology.left, 
topology.right, 1
  00:37:23 ^~~~

https://build.kde.org/view/CI%20Management/job/Dependency%20Build%20Frameworks%20kf5-qt5%20FreeBSDQt5.9/25/console

Should this be fixed with some casting?

REPOSITORY
  R278 KWindowSystem

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

To: graesslin, #frameworks, #kwin, #plasma, davidedmundson
Cc: kossebau, michaelh, ngraham

D10849: Add template for Plasma wallpaper with QML extension

[Friedrich W . H . Kossebau]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:354bd71296b5: Add template for Plasma wallpaper with QML 
extension (authored by kossebau).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10849?vs=28086=31219

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

AFFECTED FILES
  templates/CMakeLists.txt
  templates/plasma-wallpaper-with-qml-extension/CMakeLists.txt
  templates/plasma-wallpaper-with-qml-extension/Messages.sh
  templates/plasma-wallpaper-with-qml-extension/README
  templates/plasma-wallpaper-with-qml-extension/package/contents/config/main.xml
  templates/plasma-wallpaper-with-qml-extension/package/contents/ui/config.qml
  templates/plasma-wallpaper-with-qml-extension/package/contents/ui/main.qml
  templates/plasma-wallpaper-with-qml-extension/package/metadata.desktop
  
templates/plasma-wallpaper-with-qml-extension/plasma-wallpaper-with-qml-extension.kdevtemplate
  templates/plasma-wallpaper-with-qml-extension/plugin/%{APPNAMELC}plugin.cpp
  templates/plasma-wallpaper-with-qml-extension/plugin/%{APPNAMELC}plugin.h
  templates/plasma-wallpaper-with-qml-extension/plugin/CMakeLists.txt
  templates/plasma-wallpaper-with-qml-extension/plugin/qmldir

To: kossebau, mart
Cc: #frameworks, michaelh, ngraham

D11414: [xcb] Fix implementation of _NET_WM_FULLSCREEN_MONITORS

[Friedrich W . H . Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> graesslin wrote in netwm.cpp:2845-2847
> I guess we need to fix this

Thanks for having done that :)

REPOSITORY
  R278 KWindowSystem

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

To: graesslin, #frameworks, #kwin, #plasma, davidedmundson
Cc: kossebau, michaelh, ngraham

D11392: sanitizer: Move implementation into Baloo namespace

[Friedrich W . H . Kossebau]

kossebau accepted this revision.
kossebau added a comment.
This revision is now accepted and ready to land.


  Only looked at the diff, but nothing suspicious seen. If it builds and works, 
should be fine :)
  
  Perhaps mention in the commit message also that this moving into the 
namespace allows to remove DatabaseSanitizer::getDocuments() from the public 
API, so it gets more clear why this change was done and why there are also 
those changes in the patch. The reader of this commit in 2 years might be abit 
thankful :)

REPOSITORY
  R293 Baloo

BRANCH
  namespace

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

To: michaelh, #baloo, #frameworks, kossebau
Cc: kossebau, ashaposhnikov, michaelh, astippich, spoorun, nicolasfella, 
ngraham, alexeymin

D10749: Add ECMSetupQtPluginMacroNames

[Friedrich W . H . Kossebau]

This revision was automatically updated to reflect the committed changes.
Closed by commit R240:36d42640576e: Add ECMSetupQtPluginMacroNames (authored by 
kossebau).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D10749?vs=29426=29791#toc

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10749?vs=29426=29791

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

AFFECTED FILES
  docs/module/ECMSetupQtPluginMacroNames.rst
  modules/ECMSetupQtPluginMacroNames.cmake

To: kossebau, #build_system, #frameworks, apol
Cc: apol, michaelh, ngraham

D11294: Use ecm_setup_qtplugin_macro_names

[Friedrich W . H . Kossebau]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R244:8b7db6c41328: Use ecm_setup_qtplugin_macro_names 
(authored by kossebau).

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11294?vs=29427=30071

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

AFFECTED FILES
  CMakeLists.txt
  KF5CoreAddonsConfig.cmake.in

To: kossebau, #frameworks
Cc: michaelh, ngraham

D11295: Use ecm_setup_qtplugin_macro_names

[Friedrich W . H . Kossebau]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R242:f018779459e0: Use ecm_setup_qtplugin_macro_names 
(authored by kossebau).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11295?vs=29428=30074

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

AFFECTED FILES
  CMakeLists.txt
  KF5PlasmaConfig.cmake.in

To: kossebau, #plasma, #frameworks
Cc: apol, michaelh, ngraham

D11296: Use ecm_setup_qtplugin_macro_names

[Friedrich W . H . Kossebau]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R290:eb91dd14aee1: Use ecm_setup_qtplugin_macro_names 
(authored by kossebau).

REPOSITORY
  R290 KPackage

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11296?vs=29429=30073

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

AFFECTED FILES
  CMakeLists.txt
  KF5PackageConfig.cmake.in

To: kossebau, #frameworks
Cc: michaelh, ngraham

D11642: Fix the rcc binary package generation

[Friedrich W . H . Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> kossebau wrote in KF5PackageMacros.cmake:158
> This "Generating xyz" might be still good to have. Please consider picking 
> this up with a
> 
>   COMMENT "Generating: ${xyz}"
> 
> added to `add_custom_command`, where ${xyz} gets a proper variable name :) 
> and as content the relative path to the current bin dir, by some
> 
>   file(RELATIVE_PATH xyz ${CMAKE_CURRENT_BINARY_DIR} 
> ${GENERATED_RRC_CONTENTS})
> 
> or possibly using directly without the xyz var `${component}-contents.rcc` in 
> the COMMENT message.

And rather

  COMMENT "Generating ${xyz}"

so without the ":" to follow the usual pattern.

REPOSITORY
  R290 KPackage

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

To: bshah, mart, davidedmundson, apol
Cc: kossebau, #frameworks, michaelh, ngraham

D11642: Fix the rcc binary package generation

[Friedrich W . H . Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> KF5PackageMacros.cmake:158
> -include(${kpackagedir}/qrc.cmake)
> -message(STATUS \"Generating: 
> ${KDE_INSTALL_FULL_DATADIR}/${install_dir}/${root}/${component}/contents.rcc\")
> -execute_process(COMMAND ${KPACKAGE_RCC} ${kpkgqrc} --binary -o 
> ${KDE_INSTALL_FULL_DATADIR}/${install_dir}/${root}/${component}/contents.rcc 
> ERROR_VARIABLE errors RESULT_VARIABLE code)

This "Generating xyz" might be still good to have. Please consider picking this 
up with a

  COMMENT "Generating: ${xyz}"

added to `add_custom_command`, where ${xyz} gets a proper variable name :) and 
as content the relative path to the current bin dir, by some

  file(RELATIVE_PATH xyz ${CMAKE_CURRENT_BINARY_DIR} ${GENERATED_RRC_CONTENTS})

or possibly using directly without the xyz var `${component}-contents.rcc` in 
the COMMENT message.

REPOSITORY
  R290 KPackage

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

To: bshah, mart, davidedmundson, apol
Cc: kossebau, #frameworks, michaelh, ngraham

D11019: Remove left-over code from kdelibs splitting time

[Friedrich W . H . Kossebau]

kossebau created this revision.
kossebau added a reviewer: dfaure.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
kossebau requested review of this revision.

TEST PLAN
  Building/installing kinit still works, projects using
  kf5_add_kdeinit_executable() also still build

REPOSITORY
  R303 KInit

BRANCH
  removekdelibssplitcode

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

AFFECTED FILES
  KF5InitMacros.cmake

To: kossebau, dfaure
Cc: #frameworks, michaelh

D11019: Remove left-over code from kdelibs splitting time

[Friedrich W . H . Kossebau]

This revision was automatically updated to reflect the committed changes.
Closed by commit R303:3bcf7a1b3421: Remove left-over code from kdelibs 
splitting time (authored by kossebau).

REPOSITORY
  R303 KInit

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11019?vs=28595=28599

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

AFFECTED FILES
  KF5InitMacros.cmake

To: kossebau, dfaure
Cc: #frameworks, michaelh

D10848: Templates: consistent naming, fix translation catalog names & more

[Friedrich W . H . Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> apol wrote in metadata.desktop:88
> I'm not sure, if a 3rd party makes a plasmoid they would call it otherwise. 
> Are we assuming these templates are only for KDE?

Ideally the kapptemplate system would have support for an organization domain 
parameter which then could be used here (on my notes for some kapptemplatev2).

For now this brush-over is proposed with KDE community contributors in mind, to 
help them getting applets properly prepared e.g. with integration into KDE 
translations right from the begin.

Any 3rd-party which wants to keep their plasmoid 3rd-party would need to adapt 
the org namespace as desired, which I would expect them to do anyway if they 
are proud of their org.

> apol wrote in %{APPNAMELC}plugin.cpp:33
> Isn't his a rather clunky dependency there? I like templates as easy and 
> plain as possible...

Yes, not completly sure about this one.
But having seen people not getting translation setup correctly, it felt better 
to add the include and ki18n linking from the start, as removing those lines is 
easier then adding them correctly.
So I would make an exception here for ki18n. But maintainers call, just 
proposing.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kossebau, mart
Cc: apol, #frameworks, michaelh

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau updated this revision to Diff 33114.
kossebau added a comment.


  - change #pragma once to traditional include guards, not yet part of kf 
policies
  - adapt all file names to class names
  - brush over API docs

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10078?vs=33064=33114

BRANCH
  kdbusrunnerlib2

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

AFFECTED FILES
  CMakeLists.txt
  KF5DBusRunnerConfig.cmake.in
  autotests/CMakeLists.txt
  autotests/testremoterunner.cpp
  autotests/testremoterunner.h
  src/CMakeLists.txt
  src/dbusrunner/CMakeLists.txt
  src/dbusrunner/abstractrunner.cpp
  src/dbusrunner/abstractrunner.h
  src/dbusrunner/abstractrunner_p.cpp
  src/dbusrunner/abstractrunner_p.h
  src/dbusrunner/action.h
  src/dbusrunner/dbusrunner1adaptor.cpp
  src/dbusrunner/dbusrunner1adaptor_p.h
  src/dbusrunner/matchreply.cpp
  src/dbusrunner/matchreply.h
  src/dbusrunner/matchreply_p.cpp
  src/dbusrunner/matchreply_p.h
  src/dbusrunner/querymatch.h
  src/querymatch.h

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau added a comment.


  In D10078#253730 , @davidedmundson 
wrote:
  
  > > What do you think? Would give this a separate try tonight, to get some 
idea.
  >
  > Forwarding AbstractRunner::teardown is something I'd fully support.
  
  
  Forwarding `AbstractRunner::teardown` would be nice to have as well.
  
  Though I have been thinking of forwarding some (not yet existing) 
`RunnerContext::isValidChanged()` signal, once krunner has decided to no longer 
be interested in a context, due to query string having been changed by user,
  
  From my prototyping experiments I can tell that the current approach of 
recommending in API docs to 
discard-existing-request-if-new-one-arrived-as-we-assume-just-one-client-who-simply-upgraded-to-a-new-request
 feels incomplete/undecided. As implemtor I want to know which requests should 
be handled and which not.
  
  So a choice should be made:
  a) this should be codified as policy in KDBusRunner::AbstractRunner, to e.g. 
set any tracked existing MatchReply to invalid once a new request arrived,
  b) the concept of potentially parallel independent request should be 
officially supported (think e.g. stand-alone runner applets showing results in 
permanent non-popup list, updating automatically to some data source used as 
query string)
  
  In case b) it would be then good to have a way to tell which requests can be 
discarded and which should still get a full match reply.
  
  Actually I think we should hard-code a) for now, while at the same time 
leaving the option for b) once people start to have motivation to see b) 
supported.
  
  > At which point you don't need a signal in the context. IMO that's making 
things overly complex.
  > 
  > Note that the ThreadWeaver stuff in Krunner client is pretty messed up, so 
cancelling and whatnot doesn't really work as-is.
  
  Yes, the RunnerContext currently becomes suddenly invalid but without 
signalling to  the runner plugins when it happens, one has to actively query. 
But this could be fixed, given RunnerContext is a QObject.
  
  >> Using a QObject would also allow to switch from the fragile 
QVector mActiveMatchReplies to using a QPointer-based 
approach on the real MatchReply objects, which might be less
  > 
  > You can use QWeakPointer already. I don't think it's particularly needed 
though.
  
  Did not know about QWeakPointer, might have a closer look if things could be 
implemented less fragile with it.
  
  Though making MatchReply a QObject for the mentioned purpose continues to 
make sense to me, even more now.

REPOSITORY
  R308 KRunner

BRANCH
  kdbusrunnerlib2

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

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau added a comment.


  In D10078#253982 , @davidedmundson 
wrote:
  
  > > From my prototyping experiments I can tell that the current approach of 
recommending in API docs to 
discard-existing-request-if-new-one-arrived-as-we-assume-just-one-client-who-simply-upgraded-to-a-new-request
 feels incomplete/undecided
  >
  > Not really. It leaves it up to the implementor.  It covers the simple case 
well, and any advanced case is still do-able quite easily.
  
  
  ? As implementor, I can report you first hand: I want to know whether it 
makes sense to continue handling a MatchReply or if not. As in: what is the 
minimum I need to do to correctly and completely serve the requests.
  
  Having an API which has me doing  potentially useless stuff because it does 
not tell me whether something is useful to do or not, would we not call this a 
broken API?
  
  > Adding QObjects in context doesn't make sense. If you're processing stuff 
you won't process the signal.  If you're not processing things then you don't 
need the signal.
  
  Well, it makes sense once you process things in other threads with event 
loops due to further async processing, no?

REPOSITORY
  R308 KRunner

BRANCH
  kdbusrunnerlib2

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

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 43516.
kossebau added a comment.


  Updating:
  
  - bump since to 5.53
  - remove todo question about background paiting, assumed not to be required 
task of the  delegate given the current code
  - implement todo about catching destroyed user delegate (not needed by 
kdevelop, done for completeness)
  - implement setting wrappedline info in styleoption for helpevent (not needed 
by kdevelop, done for completeness)
  
  Given we are almost one week into 5.52 code development, and given that
  Dominik might find time to do some more review, I propose to delay now
  to the next release cycle, to hopefully have no objections left/raised to push
  then right after 5.52 tagging.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8708?vs=42903=43516

BRANCH
  addAnnotationItemDelegate

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

AFFECTED FILES
  src/CMakeLists.txt
  src/include/CMakeLists.txt
  src/include/ktexteditor/abstractannotationitemdelegate.h
  src/include/ktexteditor/annotationinterface.h
  src/utils/ktexteditor.cpp
  src/view/kateannotationitemdelegate.cpp
  src/view/kateannotationitemdelegate.h
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewhelpers.cpp
  src/view/kateviewhelpers.h
  src/view/kateviewinternal.h

To: kossebau, #kate, #kdevelop
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, sars

D16300: Remove double underscore (__) from header include guards

[Friedrich W. H. Kossebau]

kossebau created this revision.
kossebau added a reviewer: Kate.
Herald added projects: Kate, Frameworks.
Herald added subscribers: kde-frameworks-devel, kwrite-devel.
kossebau requested review of this revision.

REVISION SUMMARY
  Names containing a double underscore (__) are reserved to the
  C++ implementation, so better just avoid it, even with KATE namespace.
  
  Needs rename of global config include guards to avoid guard name clash.

TEST PLAN
  Still builds

REPOSITORY
  R39 KTextEditor

BRANCH
  remove__fromincludeguards

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

AFFECTED FILES
  config.h.cmake
  src/dialogs/kateconfigpage.h
  src/dialogs/katedialogs.h
  src/document/katebuffer.h
  src/inputmode/kateabstractinputmode.h
  src/inputmode/kateabstractinputmodefactory.h
  src/inputmode/katenormalinputmode.h
  src/inputmode/katenormalinputmodefactory.h
  src/inputmode/kateviinputmode.h
  src/inputmode/kateviinputmodefactory.h
  src/mode/katemodeconfigpage.h
  src/mode/katemodemanager.h
  src/mode/katemodemenu.h
  src/printing/kateprinter.h
  src/printing/printconfigwidgets.h
  src/printing/printpainter.h
  src/render/katerenderer.h
  src/schema/kateschema.h
  src/schema/kateschemaconfig.h
  src/spellcheck/spellcheckdialog.h
  src/syntax/katehighlight.h
  src/syntax/katehighlightmenu.h
  src/utils/kateautoindent.h
  src/utils/katebookmarks.h
  src/utils/katecmds.h
  src/utils/kateconfig.h
  src/utils/katedefaultcolors.h
  src/utils/kateglobal.h
  src/utils/katesedcmd.h
  src/view/kateviewhelpers.h

To: kossebau, #kate
Cc: kwrite-devel, kde-frameworks-devel, michaelh, ngraham, bruns, demsking, 
cullmann, sars, dhaumann

D16300: Remove double underscore (__) from header include guards

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D16300#345405 , @broulik wrote:
  
  > It could have just gone `#pramga once`
  
  
  conjunctive "could" -> not specified in KF policies if possible.
  
  Seems even Qt devs are undecided about it and rejected it again, so myself 
tend to avoid for now. As much as something like it is desired.

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, cullmann
Cc: broulik, cullmann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, sars, dhaumann

D16300: Remove double underscore (__) from header include guards

[Friedrich W. H. Kossebau]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:38a9214aaa0e: Remove double underscore (__) from header 
include guards (authored by kossebau).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16300?vs=43870=43871

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

AFFECTED FILES
  config.h.cmake
  src/dialogs/kateconfigpage.h
  src/dialogs/katedialogs.h
  src/document/katebuffer.h
  src/inputmode/kateabstractinputmode.h
  src/inputmode/kateabstractinputmodefactory.h
  src/inputmode/katenormalinputmode.h
  src/inputmode/katenormalinputmodefactory.h
  src/inputmode/kateviinputmode.h
  src/inputmode/kateviinputmodefactory.h
  src/mode/katemodeconfigpage.h
  src/mode/katemodemanager.h
  src/mode/katemodemenu.h
  src/printing/kateprinter.h
  src/printing/printconfigwidgets.h
  src/printing/printpainter.h
  src/render/katerenderer.h
  src/schema/kateschema.h
  src/schema/kateschemaconfig.h
  src/spellcheck/spellcheckdialog.h
  src/syntax/katehighlight.h
  src/syntax/katehighlightmenu.h
  src/utils/kateautoindent.h
  src/utils/katebookmarks.h
  src/utils/katecmds.h
  src/utils/kateconfig.h
  src/utils/katedefaultcolors.h
  src/utils/kateglobal.h
  src/utils/katesedcmd.h
  src/view/kateviewhelpers.h

To: kossebau, #kate, cullmann
Cc: cullmann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, bruns, 
demsking, sars, dhaumann

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau marked 2 inline comments as done.
kossebau added a comment.


  In D8708#344872 , @dhaumann wrote:
  
  > Feel free to push this for 5.52 (you'd have to adapt the @since 5.53 again).
  
  
  Thanks for review again :)
  
  Would prefer waiting for 5.53, for some full possible period of testing by 
people running from master, which should be at least > number of people geting 
and trying cross-project/repo patches :)
  
  > I only have minor comments that should not hold back this patch. One thing 
that pops into my eyes is that there are still some "todo" comments, which even 
talk about cornercases / bugs. Since I think you know what you are doing, I 
leave it up to you to decide wither it's ok as is or whether there need to be 
more fixes.
  
  Yes, the TODOs left are about non-critical things, like issues already in the 
old code (those about rendering group delimiters at the lines on the view 
top/bottom borders), old magic numbers which could see some reasoning, or some 
check which is practically not needed, but might be expected by some looking at 
patterns. Left as TODO remarks for my older self or someone else, in case they 
work on this, so the rough edges are documented and not silently in the code.
  
  > In any case, it was never my (and I think I can also safely say) nor any 
other's intention to hold this patch back. So yes, pinging more often is ok 
imo. Luckily, you are not a first-time contributor (who would be very much 
discouraged by our review delays), so I am sure / hope we'll see more patches 
from you in the future as well! :-)
  
  Okay, white card for more poking about reviews happily received :)
  Some other reason might also have been that I left the ""WIP" too long in the 
title, which might also have sent a wrong signal, when this was rather RFC on 
the working prototype/pre-production sample.

INLINE COMMENTS

> dhaumann wrote in kateannotationitemdelegate.h:20
> All keywords starting with __ are reserved, I suggest to remove __ at the 
> beginning and at the end.

Had those to be in line with other headers in that dir. Will do a separate 
patch to change that for the existing headers then.

REPOSITORY
  R39 KTextEditor

BRANCH
  addAnnotationItemDelegate

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

To: kossebau, #kate, #kdevelop, dhaumann
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 43868.
kossebau marked an inline comment as done.
kossebau added a comment.


  update to Dominik's last review:
  
  - remove __ from include guard
  - add comment that Option::view is always set

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8708?vs=43516=43868

BRANCH
  addAnnotationItemDelegate

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

AFFECTED FILES
  src/CMakeLists.txt
  src/include/CMakeLists.txt
  src/include/ktexteditor/abstractannotationitemdelegate.h
  src/include/ktexteditor/annotationinterface.h
  src/utils/ktexteditor.cpp
  src/view/kateannotationitemdelegate.cpp
  src/view/kateannotationitemdelegate.h
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewhelpers.cpp
  src/view/kateviewhelpers.h
  src/view/kateviewinternal.h

To: kossebau, #kate, #kdevelop, dhaumann
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, sars

D16347: Expose KTextEditor::ViewPrivate:setInputMode(InputMode) to KTextEditor::View

[Friedrich W. H. Kossebau]

kossebau resigned from this revision.
kossebau added a comment.


  Not my domain, so resigning as reviewer :)

REPOSITORY
  R39 KTextEditor

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

To: demsking, mlaurent, vkrause, dhaumann, cullmann
Cc: cullmann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, bruns, 
demsking, sars, dhaumann

D5802: ViewPrivate, KateSearchBar, KateVi::MatchHighlighter: use selection foreground for search highlights

[Friedrich W. H. Kossebau]

kossebau added a comment.


  No insight into the topic, but seeing this in my review request list 
(KDevelop):
  Given KTextEditor was ported to KSyntaxHighlighting now, what can be said 
about this patch? Does it still apply? What foreground color options are there 
now for search highlights?
  
  Or should this patch/RFC be abandoned officially, given discussion has 
petered out with no results/goals?

REPOSITORY
  R39 KTextEditor

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

To: intelfx, #kdevelop, #ktexteditor, #kate, mwolff
Cc: kossebau, cullmann, kde-frameworks-devel, brauch, dhaumann, mwolff, 
kwrite-devel, michaelh, ngraham, bruns, demsking, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:9a0505af2dbf: Introduce AbstractAnnotationItemDelegate for 
more control by consumer (authored by kossebau).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D8708?vs=43868=44806#toc

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8708?vs=43868=44806

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

AFFECTED FILES
  src/CMakeLists.txt
  src/include/CMakeLists.txt
  src/include/ktexteditor/abstractannotationitemdelegate.h
  src/include/ktexteditor/annotationinterface.h
  src/utils/ktexteditor.cpp
  src/view/kateannotationitemdelegate.cpp
  src/view/kateannotationitemdelegate.h
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewhelpers.cpp
  src/view/kateviewhelpers.h
  src/view/kateviewinternal.h

To: kossebau, #kate, #kdevelop, dhaumann
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, sars

D16770: [ftp kio-slave] Fix deletion of directory with non-latin1/utf8 parent path

[Friedrich W. H. Kossebau]

kossebau retitled this revision from "[ftp kio-slave] Fix deletion of directory 
with non-latin parent path" to "[ftp kio-slave] Fix deletion of directory with 
non-latin1/utf8 parent path".
kossebau edited the summary of this revision.
kossebau edited the test plan for this revision.

REPOSITORY
  R241 KIO

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

To: kossebau, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16770: [ftp kio-slave] Fix deletion of directory with non-latin1/utf8 parent path

[Friedrich W. H. Kossebau]

kossebau added a subscriber: aacid.
kossebau added a comment.


  @aacid Thanks for providing your ftp server, served its purpose.

REPOSITORY
  R241 KIO

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

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

D16770: [ftp kio-slave] Fix deletion of directory with non-latin parent path

[Friedrich W. H. Kossebau]

kossebau created this revision.
kossebau added a reviewer: dfaure.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
kossebau requested review of this revision.

REVISION SUMMARY
  KRemoteEncoding::directory() returns the encoded path of the directpry,
  while Ftp::ftpFolder() takes the un-encoded version.
  The implicit casting from QByteArray via QString::fromUtf8 to the QString
  type for the Ftp::ftpFolder() arg blocked the compiler from complaining about
  that coding mismatch. Possibly also non-latin1 characters being rare with
  ftp server directory layouts prevented this bug being hit so far.

TEST PLAN
  No ftp server availble with deletion rights myself, so only theoretically
  a bug seen and fixed.

REPOSITORY
  R241 KIO

BRANCH
  fixFtpDirdelete

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

AFFECTED FILES
  src/ioslaves/ftp/ftp.cpp

To: kossebau, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D16882#359595 , @rjvbb wrote:
  
  > >   Please, let's find the root causes and fix things at the base instead 
of adding such
  >
  > "we"
  
  
  (tried to not make this a "you" vs. "others" thing in the language, but yes, 
your itch only so far here, you have to scratch as a matter of normal software 
developer life)
  
  > find a fixable bug in some framework there's still no guarantee that no one 
will ever run KDevelop against a non-fixed version of that framework.
  
  _If_ it is found that the root bug is in KTextEditor, sure.
  
  For now the root is unknown, and there is some chance that it is actually the 
ctag plugin code which does something hacky to trigger the reported unexpected 
multiple parallel emissions of the menuAboutToShow signal.
  
  > I'll see if a breakpoint in and backtrace from my added if teaches me 
anything but I have my doubts (and no other ideas).
  
  I can only urge you to invest into learning how to do debugging the stuff 
that you work on. It's basic developer tooling. Like a serioius electrical 
engineer is expected to know how to use a multimeter or oscilloscope for the 
circuits they are fiddling with.
  
  >>   (even uncommented=unexplained=surprising)
  > 
  > That's easy to fix. See the "todo" comment elsewhere in the same file.
  >  TBH, I didn't add a comment yet because in itself I see nothing wrong in 
checking if you're dealing with the active view before you start adding things 
to a contextmenu.
  
  There are lots of things to check usually. But ones does not as one relies on 
callees fulfilling the explicit and implicit API contracts. And here the API 
seems to be the emission of the KTextEditor::View::contextMenuAboutToShow 
signal, that one should only be done once and for the given view where the menu 
is shown on: "Signal which is emitted immediately prior to showing the current 
context menu". 
  Adding guard code (thus complexity and stuff to maintain) to protect against 
contract breakages is only done as last resort.
  KTextEditor and kate ctags plugin are OpenSource. They can (and should) be 
fixed. Everything else is just adding debts for future code maintainers. Surely 
many humans are fine with having their fun & easy life now on the costs of 
future generation.s, but seeing myself still part of the near future kdevelop 
generation, here my banner script: "Stop creating code to work around bug 
symptoms ."

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, 
iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D16882#359888 , @rjvbb wrote:
  
  > >   _If_ it is found that the root bug is in KTextEditor, sure.
  
  
  
  
  > See https://bugs.kde.org/show_bug.cgi?id=401069#c2
  
  That looks like good work onto finding the culprit code, Please concentrate 
the related discussion there.
  
  > I don't want to delve any deeper than that into code that isn't mine and 
I'm not planning to work otherwise. This goes beyond using the CTags plugin in 
KDevelop (which is someone else's idea) and as far as that use is relevant to 
me I'm perfectly happy with a workaround (here or in KTextEditor).
  
  Please check what you wrote here. To me it tells that you are in the wrong 
place.  KDevelop, KTextEditor & Co are not for mainly having fun tinkering with 
code of an IDE, but to create a usable product, which can be easily maintained 
in snippets of free time and which is independent from one single company's 
interest.
  
  Dumping hacked-together-until-it-works solutions one does not plan to work on 
further right from the start or really care for would be quickly the death of 
this.
  
  Please tell, are you not serious about using the CTags plugin and maintaining 
the integration? I need to know why I should spent my time on reviewing this 
patch and the related issues.
  
  >>   I can only urge you to invest into learning how to do debugging the 
stuff that you work on. It's basic developer tooling. 
  > 
  > Oh, I'm pretty confident I've logged more hours in more different debuggers 
than you, more than enough to know my strengths and weaknesses.
  
  Well, I just picked up your "I suck at debugging event-driven code". If that 
is your weakness, exercise on it to improve it. It's a required skill here, 
given that lots of stuff is event-driven in kdevelop. I do not make assumptions 
about what you have done and what not (and I also resist, almost, to hint to 
quantity vs, quality ;) ).
  
  >>   >   And here the API seems to be the emission of the 
KTextEditor::View::contextMenuAboutToShow signal, that one should only be done 
once and for the given view where the menu is shown on: "Signal which is 
emitted immediately prior to showing the current context menu". 
  > 
  > That doesn't say explicitly that there will only be 1 such signal for the 
active view so this aspect could even be platform dependent.
  
  Would you agree that it is surprising to have more than 1 signal per case? So 
would we agree that this is implicitly given? What do you mean by platform 
dependent?
  
  >> generation.s, but seeing myself still part of the near future kdevelop 
generation, here my banner script: "Stop creating code to work around bug 
symptoms ."
  > 
  > Another banner would "nobody is paid to fix someone else's bugs" (except 
for a few poor sods whom I hope get paid really well for it).
  
  This bug became now also your bug as it is inhibiting your desire to enable 
the ctags plugin. Bad luck, but also normal developer life.
  
  Your work-around  would become also my work-around as kdevelop 
co-contributor, and I do not like work-arounds when they are toll code for 
being lazy now. Even more if it is someone else being lazy.

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, 
iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions

[Friedrich W. H. Kossebau]

kossebau requested changes to this revision.
kossebau added a comment.
This revision now requires changes to proceed.


  Oh, I missed the latter "For an as-yet unknown reason populateContextMenu() 
will be called with every view in which the contextmenu has been opened when 
the CTags plugin is loaded. A bug in KTextEditor maybe?"
  
  Please, let's find the root causes and fix things at the base instead of 
adding such (even uncommented=unexplained=surprising) work-arounds for 
symptoms. Saves future code readers/maintainers from wanting to toast the 
original developers of such confusing bug/misbehaviour masking code.

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, 
iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions

[Friedrich W. H. Kossebau]

kossebau added a comment.


  > TextDocument::populateContextMenu() is called when the user opens the 
contextmenu but this can happen for more than just the view currently active.
  
  What does "more than just the view currently active" mean? How can that 
happen? From what I see KTextEditor::View::contextMenuAboutToShow signal 
triggers the method. Why would there be multiple signals?

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, 
iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

D16466: [KSambaShare] Add unit test for "net usershare info" parser

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Only saw the note from @bcooksley now, pushed an intermediate disabling of 
the new test for windows, so the build at least passes: 
160df8ed7b49f68e74d30cf4343a6251ed4085d8 

  
  Seems some issue with KSambaShare having the KIOCORE_EXPORT macro set, while 
its source is also compiled into the test binary, where the macro resolves to 
import variant. No instant idea how to fix this best, so leaving to original 
code author/windows experts to sort out, sorry :)

REPOSITORY
  R241 KIO

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

To: bruns, #frameworks, broulik, apol
Cc: kossebau, bcooksley, kde-frameworks-devel, michaelh, ngraham, bruns

D16832: fix empty runner list after switching to new locale

[Friedrich W. H. Kossebau]

kossebau resigned from this revision.
kossebau added a comment.


  No time and insight, sorry.

REPOSITORY
  R308 KRunner

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

To: shaforostoff, #plasma_workspaces, davidedmundson, #frameworks
Cc: kde-frameworks-devel, michaldybczak, sdepiets, michaelh, ngraham, bruns, 
skadinna, huftis

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 42903.
kossebau added a comment.


  Improve rendering in scaled mode

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8708?vs=41828=42903

BRANCH
  addAnnotationItemDelegate

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

AFFECTED FILES
  src/CMakeLists.txt
  src/include/CMakeLists.txt
  src/include/ktexteditor/abstractannotationitemdelegate.h
  src/include/ktexteditor/annotationinterface.h
  src/utils/ktexteditor.cpp
  src/view/kateannotationitemdelegate.cpp
  src/view/kateannotationitemdelegate.h
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewhelpers.cpp
  src/view/kateviewhelpers.h
  src/view/kateviewinternal.h

To: kossebau, #kate, #kdevelop
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, bruns, 
demsking, cullmann, sars

D15582: Reference product "KF5" in widget metadata, instead of "KDE"

[Friedrich W. H. Kossebau]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R298:9c34333131b8: Reference product KF5 in widget 
metadata, instead of KDE (authored by kossebau).

REPOSITORY
  R298 KDesignerPlugin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15582?vs=41871=43174

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

AFFECTED FILES
  src/kde.widgets

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

D14530: Fix crash when save a QImage to the eps format file

[Friedrich W. H. Kossebau]

kossebau abandoned this revision.
kossebau added a comment.


  Closing this review request as the author has not reacted. D15405 
 should be serving already as more proper 
fix for the referenced bug.

REPOSITORY
  R287 KImageFormats

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

To: kossebau, mlaurent, dfaure, zccrs
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14530: Fix crash when save a QImage to the eps format file

[Friedrich W. H. Kossebau]

kossebau commandeered this revision.
kossebau edited reviewers, added: zccrs; removed: kossebau.

REPOSITORY
  R287 KImageFormats

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

To: kossebau, mlaurent, dfaure, zccrs
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau added a comment.


  @cullmann @dhaumann So, what to do? :) Do you think if we delay one more 
month you will find time to give this the wanted deeper review?
  
  Or will this continue to (understandable) lack your motivation given you are 
so far not a consumer of this new API?

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, sars

D14111: Install MIME type definition for text/x-rst ourselves for now

[Friedrich W. H. Kossebau]

kossebau abandoned this revision.
kossebau added a comment.


  I see how this is no simple matter. Given I could solve my needs locally, no 
need to add things to maintain where no-one else seems to have a need. So 
discarding, no bad feelings.
  Given that rst is around since ages and yet nobody had added that to s-m-i, 
this seems a niche need anyway :)

REPOSITORY
  R244 KCoreAddons

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

To: kossebau, dfaure, pino
Cc: fabianr, kde-frameworks-devel, michaelh, ngraham, bruns

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Thanks everyone who so far looked at this, especially @dhaumann for detailed 
comments.
  
  I am tempted to interpret the lack of further comments, especially the lack 
of principal objections as an implicit +1 on this patch as it is now :)
  
  While I still am slightly unsure about parts of the API, given no better 
ideas there I would leave it now to real life experience gathering. And then 
polishing things as found needed in any next ABI/API version.
  
  To keep things moving, I will go and merge this patch, once KF 5.51 has been 
tagged the upcoming WE, so there are 4 weeks to give the new API and also the 
default implementation some testing and considerations before getting published 
with 5.52. Of course unless there are objections raised now.

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, bruns, 
demsking, cullmann, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D8708#337408 , @dhaumann wrote:
  
  > Hm, given the size of the patch, and given it introduces new public API we 
cannot easily change, I think a silent +1 since noone reacts is not good enough.
  
  
  I agree that the amount of review done is not good enough. I would have liked 
some more feedback on both the patches, but most people sadly never got further 
than "nice feature, looking forward". I was very glad when you did your initial 
review :)
  
  On the other hand I have reiterated these patches a few times while they 
waited on phab for review, also tried multiple approaches last year before 
uploading the delegate one. So while my perfectionism-self still sees some 
issues in the new API, my pragmatism-self is actually pretty okay with it.
  The additional things I would have liked in the API actually would mean some 
rework of the currently line-based rendering approach inside `KateIconBorder`, 
which due to review needs might have worsened the chance of getting at least 
the basic feature in at all. So I dumped that work and reduced the patch to 
what we have now.
  
  > That said, I will not have time for another review until Oct 14th - so I 
would appreciate another review by someone else, if possible.
  
  Given this is API only needed for KDevelop 5.4, which might happen only begin 
of next year, I am okay with delaying another KF5 cycle/month, and schedule now 
to target 5.23 (merge after 5.22 has been tagged). If this actually means 
getting deep embracing reviews :) I was only going for action seeing that 
no-one seems to (have time to) care, before this feature is reaching it's first 
anniversary here on phabricator in November :( Not sure if I should have nagged 
more to get you to review this? But then I dislike being nagged myself for 
things which are pretty visible on phabricator, unless it's quick simple 
patches.
  
  So, anyone thinking they will have time & interest to do a full review in the 
October weeks?
  
  And I still think the annotation border feature would be a nice tool for 
people using kate/kwrite on versioned documents as well (given there is some 
VCS support to hook in). Is this really no use case Kate users have as well? -> 
https://frinring.wordpress.com/2018/09/09/from-code-to-related-bug-report-or-review-in-just-a-hover-click/
 Or do people not use Kate to work on old codebases? :P
  (a future planned addition will be integration to addressbooks/gravatar, so 
you could see the faces of the authors and/or have the option to directly 
contact them in case of need. All that could be done as one finds fancy thanks 
to the new delegate API introduced here, without having to twist something in 
KTextEditor)

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, sars

D14530: Fix crash when save a QImage to the eps format file

[Friedrich W. H. Kossebau]

kossebau added a comment.


  @zccrs Hi. Does D15405  fix things for 
you, so is this patch here no longer needed? If so, please close this review 
request by selecting and submitting the action "Abandon Revision", so it does 
no longer appear in our "Please review" list :)

REPOSITORY
  R287 KImageFormats

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

To: zccrs, mlaurent, kossebau, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions

[Friedrich W. H. Kossebau]

kossebau added a comment.


  For the record, related commits are:
  Adding the principal clean-up logic:  
R32:b837392d5f05394794a813afb7ca94e54650fcff 

  Changing it from clean-on-hide to clean-on-about-to-show-again: 
R32:4d63d74c7d189479b752a11df70afab77859a457 


REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, 
iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D16882#361733 , @rjvbb wrote:
  
  > >   `KTextEditor::View::contextMenu()` talks about "the xmlgui menu" or 
custom set "context menu object", but no hint/promise whether there is any 
instance sharing done, e.g. between views for the same document or even across 
all views in the same process(?).
  >
  > No, but from the code it's clear that there's only a single instance that 
is shared among all views. With KXMLGUI that's even unavoidable (and 
incidentally a source of problems on Mac but that's a different can of worms).
  
  
  What I meant is: in a perfect world from KDevelop side when writing code 
against KTextEditor API we would only need to look as far as the API and its 
documentation. Anything which is not specified in the API dox is implementation 
details, and the KTextEditor developers would be free to change things as they 
need, unless they break a promise given in the API dox.
  
  Thus it would be nice to have this detail specified in the API dox of 
KTextEditor, about what to expect about the lifetime of the QMenu instance and 
if it is shared and if so, between what.
  That would help both sides, the developers of KTextEditor to know what they 
are bound to, as well as users of the API to know what to prepare for/deal with.
  
  E.g. I would have expected before looking at things that each view has their 
own separate context menu instance, possibly even created on the fly per 
display :)
  
  So, "from the code it's clear" ideally would be only interesting when trying 
to find bugs. When writing code, the API should be what we look at, and not 
further. Anything else is a bug/missing feature in the API.
  
  This is orthogonal to your actually proposed bug fix, which might be the 
final fix. But right now it would be relying on internal implementation, not 
what is documented in the API (by what I quickly read).
  (sorry, myself not enough time left now, more the next week if still needed)

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: egospodinova, kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, 
antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D18167#397319 , @graesslin 
wrote:
  
  > For done code this warning is pointless and negative. I invite you to work 
with a code base like KWin where it is more important to have a working git 
blame than protection for theoretical problems. Nobody will be able to 
guarantee that a 500+ change to add override won't break. Human errors happen, 
nobody will be able to review something like that. Addressing this warning by 
adding override all over our legacy code base has a serious risk.
  >
  > I'm seriously pissed that this is forced on us and we have to change code.
  >
  > I'm totally fine with this warning for new code and new projects. But let 
projects opt in for it instead of forcing it on legacy code bases.
  
  
  @graesslin  You, I and most if not all people are not happy if things change 
around us where we were fine with the status quo, so I understand you here. 
Just, please also understand the motivation for this change, which is about 
what is the recommended default for (KDE) projects. The alternatives would be 
to not change anything ever or to stick to the lowest common denominator. Would 
you agree those are less preferable solutions?
  As a matter of fact, the (felt?) majority of maintained KDE projects 
meanwhile has been upgraded to the improved expressiveness of what the 
meanwhile rather old C++11 standards brings in terms of override (& nullptr). 
Especially as the upgrade usually can be done automated using compiler-powered 
tools, like clang-tidy, so the human factor is reduced by a good degree. My 
personal experience has been that one has rather found existing issues than 
introduced regressions. Especially with stone-age-old C++ codebases where lots 
of different lead developers had shifted designs in different directions, 
leaving some incomplete parts behind here and there accidentally when changing 
signatures of APIs.
  
  So for KWin, if no-one dares to touch the old xcb(?) codebase (which actually 
means it is not "owned" any longer, but "owns" the developers), please now do 
the opt-out from the default by explicitly disabling that compiler flag for the 
legacy codebase. Again, the settings of the ECM KDE definition macros are not 
forced on people, just set the recommended defaults.

REPOSITORY
  R240 Extra CMake Modules

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

To: aacid
Cc: kossebau, graesslin, apol, vkrause, kde-frameworks-devel, kde-buildsystem, 
michaelh, ngraham, bruns

D18434: exiv2extractor: add support for bmp, gif, webp, tga

[Friedrich W. H. Kossebau]

kossebau created this revision.
kossebau added a reviewer: Baloo.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
kossebau requested review of this revision.

REVISION SUMMARY
  Gives only minimal info for bmp, gif & tga, but knowing the size
  can be useful already.
  
  exiv2 supports bmp, gif & tga since 0.17, so for all versions since
  min required version 0.21. Webp is supported since 0.26.

REPOSITORY
  R286 KFileMetaData

BRANCH
  covergiftgabmp

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

AFFECTED FILES
  src/extractors/CMakeLists.txt
  src/extractors/exiv2extractor.cpp

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

D18450: Add extractor for AppImage files

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Example screenshot: F6561839: Screenshot_20190122_102745.png 

  
  There seems to be some bug with the Comment field though, somehow in Dolphin 
the comment is not shown, where "dump" displays it as existing.

REPOSITORY
  R286 KFileMetaData

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

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

D18450: Add extractor for AppImage files

[Friedrich W. H. Kossebau]

kossebau created this revision.
kossebau added a reviewer: Baloo.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
kossebau requested review of this revision.

REVISION SUMMARY
  Only a few properties currently can be mapped more or less to the existing
  values of the "Property" enum, ideally for the future can be extended.

TEST PLAN
  "dump" test util and Dolphin both now show the extracted metadata for
  AppImage files.

REPOSITORY
  R286 KFileMetaData

BRANCH
  addappimageextractor

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

AFFECTED FILES
  CMakeLists.txt
  src/extractors/CMakeLists.txt
  src/extractors/appimageextractor.cpp
  src/extractors/appimageextractor.h

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

D18450: Add extractor for AppImage files

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 50146.
kossebau added a comment.


  add unit test

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18450?vs=50077=50146

BRANCH
  addappimageextractor

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

AFFECTED FILES
  CMakeLists.txt
  autotests/CMakeLists.txt
  autotests/appimageextractortest.cpp
  autotests/appimageextractortest.h
  autotests/samplefiles/test.AppImage
  src/extractors/CMakeLists.txt
  src/extractors/appimageextractor.cpp
  src/extractors/appimageextractor.h

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

D18450: Add extractor for AppImage files

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D18450#398143 , @astippich 
wrote:
  
  > Can you add a test please?
  
  
  Done. The sample file sadly is some 2xx KB big, but the AppImage devs could 
not help to get it smaller without no longer being a real appimage, the runtime 
payload brings that weight.
  
  > In D18450#397920 , @kossebau 
wrote:
  > 
  >> There seems to be some bug with the Comment field though, somehow in 
Dolphin the comment is not shown, where "dump" displays it as existing.
  > 
  > 
  > That property "conflicts" with the xattr comment and is excluded in 
baloo-widgets because of that.
  
  I see. Guess it still makes sense to extract something for the Comment field, 
for any other consumers or the future.

REPOSITORY
  R286 KFileMetaData

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

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

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D18167#398076 , @graesslin 
wrote:
  
  > The human error exists as long as clang-tidy is not used. What I fear is 
that someone does a hand porting - we have seen several attempts to do that in 
KWin from various developers. If devs don't know and now fix the warnings, they 
can bring in human error.
  
  
  Okay, this experience of yours hints to me why you appear to be a bit more 
sensitive about this change.  Where myself I have never seen any issues related 
to moving to override usage, rather the opposite.
  I also understand that you have an extra level of is-this-needed protection 
when it comes to kwin due to being a core product, which one could say has paid 
out so far given the stability we all enjoy with kwin. Just...
  
  > Thus I suggest that those who think this should be the default for all 
projects by KDE do the work to run clang-tidy over the complete KDE code base 
and afterwards enable this warning.
  
  As said, it seems that most of the actively maintained codebase has already 
been moved to override usage (he, I am to "blame" for that as well) in the last 
two years or so. Which also could be seen as kind of silent support for seeing 
the non-use of override something to improve.
  
  > I'm just not happy with the approach of breaking workflow without any 
discussion at all with the larger community. We have points in time where we 
can break things. E.g. the upcoming Qt 6. What I do not like is breaking in the 
middle of a release cycle without any coordination. Also I don't want to spend 
my very little spare time hunting behind what broke KWin build. I'm really not 
pleased about this from above attitude to break the compile of projects.
  
  I would not agree on the definition of "break". This change adds a warning by 
default. Same like if some new compiler version decides to be more nasty by 
default about issues it sees. Or methods being deprecated in some newer version 
of a library.
  
  I would agree though to the point of this change being done very quickly in 
just a few days and without passing this a bit more visible by the eyes of the 
bigger developer community which relies on the defaults defined by ECM's KDE 
macros.
  Just "build-system" and "frameworks" was not really sufficient here given the 
scope it effects, and only 3(?) days between proposal and commit was also a 
very rushy, ignoring that the major community might not be able to comment 24/7 
on things.  In a perfect organisation this change of default settings would 
have been exposed some more.
  I do not think though this is "from above attitude", but more a 
"sane-to-me-and-my-circle-so-must-be-sane-to-everyone attitude" or a 
"afraid-of-potential-bikeshedding-discussion attitude"?  Not healthy in any 
case.
  
  So -1 on the execution of this change from me as well.
  
  Though then in this very case, my own take is to be pragmatic and see that 
this change makes sense in the end and that any active KDE software projects 
which have code left which should not be upgraded to C++11 and more recent 
standards should simply on their side opt-out from this warning.
  
  While talking about it, not sure what is the better approach, I have seen 
different cmake-based approaches:
  
string(REPLACE "-Wsuggest-override" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
  
check_cxx_compiler_flag("-Wno-suggest-override" HAS_WNO_SUGGEST_OVERRIDE)

if (${HAS_WNO_SUGGEST_OVERRIDE})
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-suggest-override" )
endif()
  
  What would cmake professionals use here?

REPOSITORY
  R240 Extra CMake Modules

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

To: aacid
Cc: zzag, davidedmundson, kossebau, graesslin, apol, vkrause, 
kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D18167#398360 , @aacid wrote:
  
  > In D18167#398343 , @kossebau 
wrote:
  >
  > > only 3(?) days between proposal and commit was also a very rushy
  >
  >
  > Check your dates better please, it's 9 days
  
  
  Oops, indeed, my bad, somehow my eyes slipped one line from the data of the 
"Closed by commit" to  the date of the announcement "will push next saturday". 
No coffee yet perhaps when I wrote that, the "(?)" hints I was at least unsure 
;) But yes, blame taken.
  
  Still, also 9 days I would consider a too short period for central setting 
changes. It has not been an urgent change.

REPOSITORY
  R240 Extra CMake Modules

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

To: aacid
Cc: zzag, davidedmundson, kossebau, graesslin, apol, vkrause, 
kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D17086: Add icons for application-vnd.appimage

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D17086#363855 , @ngraham wrote:
  
  > Thanks for the icon! I'd recommend mimicking the style of the other package 
icons that have a symbol:
  >
  > In other words, add a zipper towards the right-side,, put the AppImage 
symbol in the bottom-left corner, and make the small version just be a line-art 
version of the AppImage logo.
  
  
  I considered that. But decided in the initial proposal against, with this 
reasoning:
  AppImage files are different to distribution packages, they will not be 
unpackaged or untarred in any way to be deployed into the system, at least to 
what the user will experience. But will be used as is. Same like a ODT file is 
actually a ZIP file, but the nature of being something tarred and composed from 
lots of different "files" is not exposed to the user.
  So the semantics of a zipper would not really match that much, unless one 
looks at it from a pure file creation POV.
  
  I rather think the zipper might misguide people into thinking AppImages are 
the same category as debs, rpms & Co.
  
  With that in mind, what do you think?

REPOSITORY
  R266 Breeze Icons

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

To: kossebau, #vdg, ngraham
Cc: ngraham, kde-frameworks-devel, michaelh, bruns

D17086: Add icons for application-vnd.appimage

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Open questions:
  
  - how to support the dark icon set?
  - what would be better colors?
  
  Example of icon use with 64 & 16 pixel size, and bigger in preview:
  F6435751: Screenshot_20181121_220325.png 

  
  The icon also fixes a small offset issue in the original logo :) cmp. the one 
on https://appimage.org/

REPOSITORY
  R266 Breeze Icons

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

To: kossebau, #vdg
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17086: Add icons for application-vnd.appimage

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 46231.
kossebau added a comment.


  - add symlinks for application-x-iso9660-appimage to application-vnd.appimage

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17086?vs=46229=46231

BRANCH
  addappimageiconv2

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

AFFECTED FILES
  icons/mimetypes/16/application-vnd.appimage.svg
  icons/mimetypes/16/application-x-iso9660-appimage.svg
  icons/mimetypes/22/application-vnd.appimage.svg
  icons/mimetypes/22/application-x-iso9660-appimage.svg
  icons/mimetypes/32/application-vnd.appimage.svg
  icons/mimetypes/32/application-x-iso9660-appimage.svg
  icons/mimetypes/64/application-vnd.appimage.svg
  icons/mimetypes/64/application-x-iso9660-appimage.svg

To: kossebau, #vdg, ngraham
Cc: TheAssassin, ngraham, kde-frameworks-devel, michaelh, bruns

D17086: Add icons for application-vnd.appimage

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D17086#363977 , @ngraham wrote:
  
  > We have two options here:
  >
  > 1. Follow the style and make the AppImage icon look like a document with a 
folded-over corner that has an the AppImage logo  in the middle
  > 2. Engage #VDG  and Come up a new 
icon style for executables, and then make the AppImage icon in that new style
  >
  >   [...] Feel free to do #1 for now.
  
  
  Sadly no time currently to engage in such discussion, so quickly did #1 to 
have something for now.
  
  In D17086#364095 , @TheAssassin 
wrote:
  
  > The dark blue is quite nice, but I thought perhaps a color from the 
official AppImage logo might fit better.
  
  
  I guess one should stick to the Breeze color palette for the icons, for 
consistent look. Other logos are also matched to closes Breeze palette color 
from what I quickly looked at, But exactly sure.
  
  > The logo could by the way also be used for the type 1 MIME type, 
`application/x-iso9660-appimage` and `application/x-appimage` IIRC. 
  >  You might also add a `1` somewhere in there to indicate the type, but as 
both are still relevant and should mostly work equally, that's not really 
necessary.
  
  No clue yet about the "application/x-iso9660-appimage" MIME type, what is the 
difference?
  "application/x-appimage" is an outdated alias to "application/vnd.appimage", 
no? So that should be resolved by the alias logic of shared-mime-info at the 
core, no need for explicit own icon entry,

REPOSITORY
  R266 Breeze Icons

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

To: kossebau, #vdg, ngraham
Cc: TheAssassin, ngraham, kde-frameworks-devel, michaelh, bruns

D17086: Add icons for application-vnd.appimage/x-iso9660-appimage

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D17086#366120 , @ngraham wrote:
  
  > Looks great to me then!
  >
  > But how do I make this work? I compiled and installed the icon and verified 
that it's installed in the right place. But my AppImage isn't getting the new 
icon, not even after running `update-desktop-database`.
  >
  > F6441744: Weird.png 
  >
  > What am I doing wrong here?
  
  
  There might be the chance that the icon cache is not updated autpmagically 
after the icon installation. Try removing `.cache/icon-cache.kcache` before 
running a new instance of dolphin. Not sure if some index counter needs to be 
changed as welll for new icons, could not find some related change on new icon 
commits.

REPOSITORY
  R266 Breeze Icons

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

To: kossebau, #vdg, ngraham
Cc: TheAssassin, ngraham, kde-frameworks-devel, michaelh, bruns

D17086: Add icons for application-vnd.appimage/x-iso9660-appimage

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D17086#366111 , @ngraham wrote:
  
  > Thanks, that looks great! As for the background color, I'd say it's okay to 
use a different shade of blue if it would better match the AppImage branding.
  
  
  For now I favor breeze palette usage, given the same IIRC is also done 
elsewhere with brand logo usage in Breeze icons.
  
  > Don't worry about participating in the style change discussion; if that 
ever bears any fruit, we'll change this icon as a part of it.
  
  Happy to have SVG editing skilled people take over, Inkscape is really in my 
way here :)
  
  > The images I posted came from the Cuttlefish app:
  >  F6441674: Cuttlefish.png 
  
  Ha, I noticed the very same time it be just screenshots snippets from 
cuttlefish when I looked at it during task-related usage :) Thanks for sharing 
the how.
  Might be a nice feature addition to have cuttlefish create such summary 
renderings as pixmaps itself to copy here & elsewhere, without the need to go 
via screenshot tool? Noted for fun hacks times...

REPOSITORY
  R266 Breeze Icons

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

To: kossebau, #vdg, ngraham
Cc: TheAssassin, ngraham, kde-frameworks-devel, michaelh, bruns

D17086: Add icons for application-vnd.appimage/x-iso9660-appimage

[Friedrich W. H. Kossebau]

kossebau retitled this revision from "Add icons for application-vnd.appimage" 
to "Add icons for application-vnd.appimage/x-iso9660-appimage".

REPOSITORY
  R266 Breeze Icons

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

To: kossebau, #vdg, ngraham
Cc: TheAssassin, ngraham, kde-frameworks-devel, michaelh, bruns

D17086: Add icons for application-vnd.appimage

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 46229.
kossebau added a comment.


  - use document-style icon shape, to match other existing icons for executables
  - use Breeze palette color that is more close to AppImage logo one

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17086?vs=45976=46229

BRANCH
  addappimageiconv2

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

AFFECTED FILES
  icons/mimetypes/16/application-vnd.appimage.svg
  icons/mimetypes/22/application-vnd.appimage.svg
  icons/mimetypes/32/application-vnd.appimage.svg
  icons/mimetypes/64/application-vnd.appimage.svg

To: kossebau, #vdg, ngraham
Cc: TheAssassin, ngraham, kde-frameworks-devel, michaelh, bruns

D17086: Add icons for application-vnd.appimage

[Friedrich W. H. Kossebau]

kossebau added a comment.


  F6441665: Screenshot_20181126_014024.png 
 for an updated example usage.
  
  @ngraham How do you create those summary pictures you used above for the 
comparison previews?
  
  In D17086#366097 , @TheAssassin 
wrote:
  
  > The new logos look great!
  
  
  Happy to learn you like them :)
  
  In D17086#366099 , @TheAssassin 
wrote:
  
  > AFAIK `vnd.appimage` resolves to type 2, whereas `x-[iso9660-]appimage` 
resolves to type 1. At least that's the MIME data I've been working with all 
the time so far.
  
  
  Ah, so alias with the other then, okay. For a start I guess that giving 
"application/x-iso9660-appimage" the same icon might work (using the icon 
shared linking system), not sure if there needs to be any indication in the 
icon itself about the type, also no idea how to mark type 1 and how type 2.

REPOSITORY
  R266 Breeze Icons

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

To: kossebau, #vdg, ngraham
Cc: TheAssassin, ngraham, kde-frameworks-devel, michaelh, bruns

D17086: Add icons for application-vnd.appimage/x-iso9660-appimage

[Friedrich W. H. Kossebau]

This revision was automatically updated to reflect the committed changes.
Closed by commit R266:e3266db59d2f: Add icons for 
application-vnd.appimage/x-iso9660-appimage (authored by kossebau).

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17086?vs=46231=46359

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

AFFECTED FILES
  icons/mimetypes/16/application-vnd.appimage.svg
  icons/mimetypes/16/application-x-iso9660-appimage.svg
  icons/mimetypes/22/application-vnd.appimage.svg
  icons/mimetypes/22/application-x-iso9660-appimage.svg
  icons/mimetypes/32/application-vnd.appimage.svg
  icons/mimetypes/32/application-x-iso9660-appimage.svg
  icons/mimetypes/64/application-vnd.appimage.svg
  icons/mimetypes/64/application-x-iso9660-appimage.svg

To: kossebau, #vdg, ngraham, TheAssassin
Cc: TheAssassin, ngraham, kde-frameworks-devel, michaelh, bruns

D17086: Add icons for application-vnd.appimage/x-iso9660-appimage

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Thanks for feedback/review :)

REPOSITORY
  R266 Breeze Icons

BRANCH
  addappimageiconv2

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

To: kossebau, #vdg, ngraham, TheAssassin
Cc: TheAssassin, ngraham, kde-frameworks-devel, michaelh, bruns

D17088: [thumbnailer appimage] Fix building with libappimage not in system path

[Friedrich W. H. Kossebau]

kossebau created this revision.
kossebau added reviewers: broulik, TheAssassin, azubieta.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.
kossebau requested review of this revision.

REVISION SUMMARY
  The current CMake Config file of libappimage does not specify any
  imported target nameed "appimage". Instead it provides a shared lib
  target "libappimage" and a static lib target "libappimage_static".
  Both though are also broken in that they have targets to further
  dependencies in their link list, which though are only defined in the
  internal build system, but not provided with the installed CMake Config
  file.
  
  Additionally the LIBAPPIMAGE_INCLUDE_DIRS is currently not set,
  https://github.com/AppImage/libappimage/pull/17 hopefully fixes this
  for future versions.
  
  The previous simple "appimage" only worked if libappimage was installed
  to an otherwise known prefix, so include dirs and library paths were among
  the ones considered, and the linker would find libappimage via -lappimage.

TEST PLAN
  Building against current version of libappimage installed to custom prefix
  works, as well as building against the patched version.

REPOSITORY
  R320 KIO Extras

BRANCH
  fixappimagelinking

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

AFFECTED FILES
  thumbnail/CMakeLists.txt

To: kossebau, broulik, TheAssassin, azubieta
Cc: kde-frameworks-devel, kfm-devel, alexde, sourabhboss, feverfew, michaelh, 
spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, 
mikesomov

D17088: [thumbnailer appimage] Fix building with libappimage not in system path

[Friedrich W. H. Kossebau]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R320:6c2ebbb5b853: [thumbnailer appimage] Fix building with 
libappimage not in system path (authored by kossebau).

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17088?vs=45983=46055

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

AFFECTED FILES
  thumbnail/CMakeLists.txt

To: kossebau, broulik, TheAssassin, azubieta
Cc: kde-frameworks-devel, kfm-devel, alexde, sourabhboss, feverfew, michaelh, 
spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, 
mikesomov

D17044: Add FindExiv2.cmake to ECM

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> FindExiv2.cmake:31
> +# Since 5.53.0.
> +# TODO KF6: Rename to FindLibExiv2.cmake
> +#

Could you not already do this now, before it's first released?

Any potential users currently have their own copy, they will not use this 
version.
Once they switch to rely on the version provided by ECM (and remove their own 
copy), they could also adapt the naming.
New users could use the new name from the beginning.

REPOSITORY
  R240 Extra CMake Modules

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

To: cgiboudeaux, apol, dfaure
Cc: kossebau, rempt, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, 
bruns

D17015: Fix the Qt doc creation with Qt 5.12.

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Good to see you caring for ECM documentation not getting broken with Qt 5.12 
:)
  
  Any idea how we could perhaps deduplicate the  FindQHelpGenerator.cmake with 
the one from find-modules (which is a helper for runtime with the ECMAddQch 
macro)? No instant idea yet, perhaps also better to have dedicated variants for 
each purppse? Needs me another round of thinking.

INLINE COMMENTS

> ECMQueryQmake.cmake:6
> +else()
> +find_program(_qmake_executable_default NAMES qmake-qt5)
>  endif()

Perhaps worth to add a comment describing all known scenarios why we reach the 
else branch.
The version with find_package(Qt5Core) failing due to NONE language would be 
not on my radar, thus nice to have it listed here for future code readers.

REPOSITORY
  R240 Extra CMake Modules

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

To: cgiboudeaux, kossebau
Cc: kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D16938: FindQHelpGenerator: try to find Qt5Help instead of Qt5Core

[Friedrich W. H. Kossebau]

kossebau accepted this revision.
kossebau added a comment.
This revision is now accepted and ready to land.


  Good find. No idea why it was not like this from the start.
  
  Untested (besides grepping my local Qt 5.11 CMake Config files to confirm 
myself that's really the place where the imported target is added), but given 
you tested pretty sure this is a proper fix. Thanks.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  find-qhelpgenerator-target

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

To: pino, kossebau
Cc: kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D16882#360857 , @rjvbb wrote:
  
  > Re-opening because I found an actual flaw in KDevelop after noticing that 
context menu duplication still occurred when only the active view receives the 
aboutToShowContextMenu signal.
  >
  > The `addedContextMenu` member exists because `"we want to remove the added 
stuff when the menu hides"`. This should of course read "when the menu reopens 
in again, possibly in a different view".
  
  
  Good find, that seems indeed broken.
  
  > The flaw here is that the design forgets that the context menu instance is 
shared among views. It expects `d->addedContextMenu` to exist and contain the 
QMenu added by a previous view, but this cannot be the case in the current 
implementation where the variable is only allocated when the menu is first 
opened in a given view.
  
  I would see the flaw also in that there is no specification in the 
KTextEditor API how the context menu is shared/reused. 
`KTextEditor::View::contextMenu()` talks about "the xmlgui menu" or custom set 
"context menu object", but no hint/promise whether there is any instance 
sharing done, e.g. between views for the same document or even across all views 
in the same process(?).
  
  > If the `addedContextMenu` is to be removed in JIT-fashion before reopening 
the context menu, it should be a static variable.
  
  That might be a way, yes.
  
  Before though I would like to have it first sorted out with the KTextEditor 
people what to expect here and whether the API dox could resolve that 
undefinedness. Given the current implementation kdevelop-side I would not be 
surprised if KTextEditor changed implementation here, but needs to be explored. 
The proposed fix relies on the current implementation, which is a bit fragile.
  
  Another option might be to link up to the menu being closed and clean up 
then, as the comment on the `addedContextMenu` member claimed. (personally 
preferred to clean up right after use). 
  Also needs to be explored if there once was such an implementation and why it 
has been removed. Might do in the next days.

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, 
iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

D18088: FindGperf: in ecm_gperf_generate set SKIP_AUTOMOC for generated file

[Friedrich W. H. Kossebau]

kossebau created this revision.
kossebau added reviewers: Frameworks, pino.
Herald added projects: Frameworks, Build System.
Herald added subscribers: kde-buildsystem, kde-frameworks-devel.
kossebau requested review of this revision.

REVISION SUMMARY
  Avoids to have manually set the property on the generated source file to
  deal with changed CMake policy CMP0071.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  skipautomoxforgperf

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

AFFECTED FILES
  find-modules/FindGperf.cmake

To: kossebau, #frameworks, pino
Cc: kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18088: FindGperf: in ecm_gperf_generate set SKIP_AUTOMOC for generated file

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Not sure if there ever s a chance somebody would inject QObject code into 
such a generated file?

REPOSITORY
  R240 Extra CMake Modules

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

To: kossebau, #frameworks, pino
Cc: kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D15513: Fix dangling reference with "auto" becoming "QStringBuilder"

[Friedrich W. H. Kossebau]

kossebau created this revision.
kossebau added reviewers: ivan, mlaurent.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
kossebau requested review of this revision.

REVISION SUMMARY
  With QT_USE_QSTRINGBUILDER being enabled, code best is checked with
  clazy -checks=auto-unexpected-qstringbuilder for these traps now enabled

REPOSITORY
  R6 KActivities

BRANCH
  fixqstringliteralvsauto

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

AFFECTED FILES
  src/imports/activitymodel.cpp

To: kossebau, ivan, mlaurent
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15513: Fix dangling reference with "auto" becoming "QStringBuilder"

[Friedrich W. H. Kossebau]

This revision was automatically updated to reflect the committed changes.
Closed by commit R6:ea5416f8f99a: Fix dangling reference with auto 
becoming QStringBuilder (authored by kossebau).

REPOSITORY
  R6 KActivities

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15513?vs=41661=41668

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

AFFECTED FILES
  src/imports/activitymodel.cpp

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

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 41694.
kossebau added a comment.


  fix crash due to (no longer needed) circular dependencies on view object
  constrctions

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8708?vs=41225=41694

BRANCH
  addAnnotationItemDelegate

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

AFFECTED FILES
  src/CMakeLists.txt
  src/include/CMakeLists.txt
  src/include/ktexteditor/abstractannotationitemdelegate.h
  src/include/ktexteditor/annotationinterface.h
  src/view/kateannotationitemdelegate.cpp
  src/view/kateannotationitemdelegate.h
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewhelpers.cpp
  src/view/kateviewhelpers.h
  src/view/kateviewinternal.h

To: kossebau, #kate, #kdevelop
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D15582: Reference product "KF5" in widget metadata, instead of "KDE"

[Friedrich W. H. Kossebau]

kossebau created this revision.
kossebau added a reviewer: Frameworks.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
kossebau requested review of this revision.

REPOSITORY
  R298 KDesignerPlugin

BRANCH
  kdeiskf5now

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

AFFECTED FILES
  src/kde.widgets

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

D15591: Add Open Document thumbnailer

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Just FYI, as I was added, I currently have no time reserved for document 
related code work:
  
  There is such a thumbnailer for OpenDocument Format document which exposes 
the preview/thumbnail picture from the data since ages on what now is 
store.kde.org, here my old bookmark:
  https://www.opendesktop.org/p/1081271
  
  IIRC it is packaged by a few, at least was for openSUSE.
  
  The Calligra thumbnailer has an advantage over the simple thumbnail 
extracting one here: it actually understands the content of the file (when it 
comes to standard ODF) and can render to bigger previews, as much as needed, 
not just the max isze of the embedded thumbnail (IIRC 128x128 in the ODF 
specs).  Or for OpenDocument Formats files which were generated with no 
preview/thumbnail added.
  
  Sadly the KDE thumbnail system has no priority system, so more powerful 
thumbnailer if installed could overrule less powerful ones.
  
  The Krita format spec has a preview picture in the full size IIRC, there 
scaling is no issue.

REPOSITORY
  R320 KIO Extras

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

To: broulik, #frameworks, #vdg, ngraham, kossebau, jtamate
Cc: leinir, kossebau, jtamate, ngraham

D15405: [EPS] Fix crash at app shutdown (being tried to persist clipboard image)

[Friedrich W. H. Kossebau]

This revision was automatically updated to reflect the committed changes.
Closed by commit R287:98c65a438dfb: [EPS] Fix crash at app shutdown (being 
tried to persist clipboard image) (authored by kossebau).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D15405?vs=41341=41808#toc

REPOSITORY
  R287 KImageFormats

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15405?vs=41341=41808

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

AFFECTED FILES
  src/imageformats/eps.cpp

To: kossebau, zccrs, dfaure, pino
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau marked 15 inline comments as done.
kossebau added inline comments.

INLINE COMMENTS

> dhaumann wrote in abstractannotationitemdelegate.h:52
> I am asking myself: Is wrappedLineCount >= 1 always true? If so, can you add 
> this as documentation? wrappedLineCount == 1 means no wrapping line?

Yes. I see now that this can be confusing and semantically strange, if 
wrappedLineCount == 1 means actually no wrapping here. Unsure what ould be a 
better way to express this. One ould use the same name and define 0 to mean 
that no wrapping is done. And 1 would just be a bogus value? Proposals very 
welcome.

> dhaumann wrote in abstractannotationitemdelegate.h:65
> Is this to be set by the user, and if so, is there any special meaning to a 
> default-constructed QSize()?

This parameter was inspired by QStyleOptionViewItem::decorationSize. I imagined 
it could be e.g. used if one ever starts to show avatar icons for commit 
authors in the annotations, or for other icon-based indicators which could be 
shown (bug fix, reviewed, etc).

There is no specification on the meaning of an invalid QSize with 
QStyleOptionViewItem::decorationSize 
. I would 
tend to leave this here unspecified as well for now, until this gets in real 
use or the QStyleOptionViewItem one gets specified?

> dhaumann wrote in abstractannotationitemdelegate.h:80
> Is this a bitmask? The << 0, <<1, <<2 notation implies that it is.

Yes, as a line could be both begin and end of an annotation grouping (if both 
neighbour lines belong to different groups). Of course InGroup is mutually 
exclusive to GroupBegin and GroupEnd. IIRC (been some time since Nob 2017 :) ) 
I had used normal enum values, with another value GroupBeginAndEnd. But the 
resulting code using the enum was more complicated.
Not sure these days, perhaps I should retry. Need to recheck what similar 
enumas there are in the Qt world, so at least things are consistent.

> dhaumann wrote in abstractannotationitemdelegate.h:95
> You export the class, but then the constructors are inlined. Could you please 
> move the implementation to the cpp file? That leaves us more room to fixes by 
> shipping an updated version of the library.

Okay. I had looked at the other interface classes, those I looked at, even if 
QObject-based, are header-only, that's why it was done like this. Will change, 
as I remember from other projets e,g. Windows has issues with this.

> dhaumann wrote in kateannotationitemdelegate.cpp:52-53
> Given you check for a valid pointers here, would it also be an option to pass 
> references? Or would that violate Qt style API?

It is following QAbstractItemDelegate ::paint(...) 
 signature here. So I 
would lean to stay with the current code. But as you prefer.

> dhaumann wrote in kateannotationitemdelegate.h:2-4
> Is this copyright really correct?

Indeed for the headerthere is no code copied over, well, besides the API being 
inspired by QAbstractItemDelegate. Reduced to mine.

> dhaumann wrote in kateannotationitemdelegate.h:9
> This is v2 only, I think this should be avoided at all costs... We try to get 
> to v2+... I think you can change this, since this is your work. Even if it's 
> based on others, the work of others is in the other files.

Changed this for the header and also for the source. While the initial code was 
copied over from kateviewhelpers.cpp (thus the complete license header), 
comparing it now to the original, I would think it has been that much rewritten 
to match the delegate API, that it safely can be called a copyrightable product 
of its own, with no substantial parts of the original left (where not required 
by the general Qt API).

> dhaumann wrote in kateviewhelpers.cpp:2636
> Do you really need the timer here? I thought update() goes through the event 
> queue anyways?

I copied existing code here, for consistency. No really sure when to call it 
directly and when to go via  event loop.

> dhaumann wrote in kateviewhelpers.cpp:2646
> 2nd time this comment pops up. Do you have an answer? In Qt, this would be an 
> extra setAutoFillBackgroundEnabled(bool) , or something similar... In any 
> case - this needs to be decided before the interface goes live :-)

Yes, this is a TODO question to you Kate developers :)
Currently KateIconBorder::paintBorder() paints the background per line with

  p.fillRect(0, y, w - 5, h, m_view->renderer()->config()->iconBarColor());

before starting to draw the annotation stuff and icons, which also cares for 
the displayed lines with no content.
I am unsure whether the delgate should be asked to take responsibility to care 
for at least blanking the view, or if it should assume some default background 
has been already painted.
QStyleOptionViewItem has a property `backgroundBrush` which seems to be expeted 
to be used by QAbstractItemDelegate, so if following that API completley, we 

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 41778.
kossebau marked 3 inline comments as done.
kossebau added a comment.


  Update to Dominik's first review

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8708?vs=41694=41778

BRANCH
  addAnnotationItemDelegate

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

AFFECTED FILES
  src/CMakeLists.txt
  src/include/CMakeLists.txt
  src/include/ktexteditor/abstractannotationitemdelegate.h
  src/include/ktexteditor/annotationinterface.h
  src/utils/ktexteditor.cpp
  src/view/kateannotationitemdelegate.cpp
  src/view/kateannotationitemdelegate.h
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewhelpers.cpp
  src/view/kateviewhelpers.h
  src/view/kateviewinternal.h

To: kossebau, #kate, #kdevelop
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D8708#326590 , @dhaumann wrote:
  
  > What I would like to see is a comment // KF6: Merge 
KTextEditor::AnnotationViewInterfaceV2 into 
KTextEditor::AnnotationViewInterface (kossebau).
  >  For me, this comment is really important, since this tells you that you 
will in 2-3 years (when Qt6 arrives) work on this and merge it down: Since 
there is only you (KDevelop) who is using this interface, so you have to 
maintain it ;)
  
  
  Forgot that in the previous update, now added. Curious though why you do not 
expect Kate to offer users some similar annotation display experience one day 
if working on versioned text documents ;)

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 41828.
kossebau added a comment.


  also add todo about merging KTextEditor::AnnotationViewInterfaceV2 into 
KTextEditor::AnnotationViewInterface

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8708?vs=41778=41828

BRANCH
  addAnnotationItemDelegate

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

AFFECTED FILES
  src/CMakeLists.txt
  src/include/CMakeLists.txt
  src/include/ktexteditor/abstractannotationitemdelegate.h
  src/include/ktexteditor/annotationinterface.h
  src/utils/ktexteditor.cpp
  src/view/kateannotationitemdelegate.cpp
  src/view/kateannotationitemdelegate.h
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewhelpers.cpp
  src/view/kateviewhelpers.h
  src/view/kateviewinternal.h

To: kossebau, #kate, #kdevelop
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars

D18434: exiv2extractor: add support for bmp, gif, webp, tga

[Friedrich W. H. Kossebau]

kossebau added a reviewer: Dolphin.

REPOSITORY
  R286 KFileMetaData

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

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

D18450: Add extractor for AppImage files

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 50073.
kossebau added a comment.


  - switch and use any localized versions found matching the current system 
locale, should be more expected
  - also extract appdata  and expose as plain text, even though 
that can be quite some data, but that's the UI's task to handle that

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18450?vs=50050=50073

BRANCH
  addappimageextractor

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

AFFECTED FILES
  CMakeLists.txt
  src/extractors/CMakeLists.txt
  src/extractors/appimageextractor.cpp
  src/extractors/appimageextractor.h

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

D18450: Add extractor for AppImage files

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 50077.
kossebau added a comment.


  - skip getting the unlocalized description if there is a localized one

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18450?vs=50073=50077

BRANCH
  addappimageextractor

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

AFFECTED FILES
  CMakeLists.txt
  src/extractors/CMakeLists.txt
  src/extractors/appimageextractor.cpp
  src/extractors/appimageextractor.h

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

D18434: exiv2extractor: add support for bmp, gif, webp, tga

[Friedrich W. H. Kossebau]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:6bc922351db0: exiv2extractor: add support for bmp, gif, 
webp, tga (authored by kossebau).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18434?vs=50016=50400

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

AFFECTED FILES
  src/extractors/CMakeLists.txt
  src/extractors/exiv2extractor.cpp

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