Re: We need to remove exec_program() from our Cmake files

[Milian Wolff]

On Donnerstag, 26. Oktober 2023 07:27:54 CEST Johnny Jazeix wrote:
> Hi,
> 
> for the list:
> https://lxr.kde.org/search?%21v=kf5-qt5&_filestring=&_string=exec_program&_c
> asesensitive=1

A lot of these should be trivial to port. For `chmod` e.g. there's now 
`file(CHMOD ...)` [1], similarly there's also `file(REMOVE ...)`. Anything 
related to pkgconfig should go through `pkg_check_modules` [2]. And for the 
remaining bits, using `execute_process` should be straight forward I hope.

Cheers

[1]: https://cmake.org/cmake/help/latest/command/file.html#chmod
[2]: https://cmake.org/cmake/help/latest/module/FindPkgConfig.html
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: We need to remove exec_program() from our Cmake files

[Milian Wolff]

On Donnerstag, 26. Oktober 2023 07:27:54 CEST Johnny Jazeix wrote:
> Hi,
> 
> for the list:
> https://lxr.kde.org/search?%21v=kf5-qt5&_filestring=&_string=exec_program&_c
> asesensitive=1

A lot of these should be trivial to port. For `chmod` e.g. there's now 
`file(CHMOD ...)` [1], similarly there's also `file(REMOVE ...)`. Anything 
related to pkgconfig should go through `pkg_check_modules` [2]. And for the 
remaining bits, using `execute_process` should be straight forward I hope.

Cheers

[1]: https://cmake.org/cmake/help/latest/command/file.html#chmod
[2]: https://cmake.org/cmake/help/latest/module/FindPkgConfig.html
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: How to get dependencies into freebsd ci?

[Milian Wolff]

On Samstag, 16. September 2023 22:00:37 CEST Ben Cooksley wrote:
> On Sun, Sep 17, 2023 at 3:16 AM Milian Wolff  wrote:
> > Hey all,
> 
> Hi Milian,
> 
> > While looking at the kate CI setup, I saw that it gets build on freebsd. I
> > would like to get that coverage too for heaptrack, but when I try to add
> > 
> > https://invent.kde.org/sysadmin/ci-utilities/raw/master/gitlab-templates/
> > freebsd.yml
> > 
> > Then heaptrack fails to find the elfutils dependency. This works on linux,
> > but
> > I have zero clue how that is done.
> > 
> > Can I add dependencies covered by `craft` to `.kde-ci.yml`?
> 
> Dependencies are managed in different ways depending on the platform in
> question.
> 
> For Linux, Android and Windows builds, this starts with a Docker image that
> can be found at https://invent.kde.org/sysadmin/ci-images
> You can have dependencies added to those by filing a merge request against
> that repository.
> 
> As you'll see, in the case of Linux we source just about everything from
> the distro package manager (in our case, it's SUSE Tumbleweed).
> For Android and Windows, the majority of the dependencies come from Craft.
> 
> FreeBSD is the exception to all of this, as those builders are fixed
> permanent machines rather than ephemeral containers that are only around
> for a single build.
> For these, please file a Sysadmin ticket.
> 
> > Where can I find documentation on what to put in there? Furthermore, is
> > there
> > some best practices when it comes to CI configuration for KDE projects? By
> > chance I found https://community.kde.org/Infrastructure/GitLab/CI/
> > Static_Code_Analysis which is interesting - is there more like it
> > somewhere?
> 
> I'm afraid we've not done a terribly good job at documenting things,
> however if you are using the templates available at
> https://invent.kde.org/sysadmin/ci-utilities/-/tree/master/gitlab-templates
> then i'd refer you to
> https://invent.kde.org/sysadmin/ci-utilities/-/blob/master/config-template.y
> ml?ref_type=heads for the options that the system supports. Note that these
> aren't used by the craft-* or flatpak templates as those are CD jobs rather
> than CI jobs.

Thanks. Where can I find out what these options actually do? `force-inject-
asan` sounds interesting.

Generally, is it possible to compile and run tests with asan enabled - ideally 
configured through a CMakePreset?

> The Static Code Analysis job referred to there is based on legacy Jenkins
> infrastructure which has been shutdown and it therefore will no longer
> function, however the new system provides analysis jobs that work with
> cppcheck to provide similar functionality if enabled.

So the wiki page is obsolete and should be removed?

And cppcheck is nice, but it's a long shot from clang-tidy and clazy. I 
understand that's the way it is for now and it's not possible to get 
additional checks enabled for projects?

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: How to get dependencies into freebsd ci?

[Milian Wolff]

On Samstag, 16. September 2023 17:47:28 CEST Tobias C. Berner wrote:
> Moin moin
> 
> I now installed elfutils-0.187 on the FreeBSD builders.

Thanks Tobias!

Can you help me a bit more please? Apparently libunwind is found - including 
support for unw_backtrace - yet I still run into a compile error later on, 
see:

https://invent.kde.org/sdk/heaptrack/-/jobs/1196913

```
-- Performing Test LIBUNWIND_HAS_UNW_BACKTRACE
-- Performing Test LIBUNWIND_HAS_UNW_BACKTRACE - Success
...
/var/tmp/gitlab_runner/builds/TY-7Z6G7M/0/sdk/heaptrack/src/track/
trace_libunwind.cpp:67:12: error: use of undeclared identifier 'unw_backtrace'
return unw_backtrace(data, MAX_SIZE);
   ^
1 error generated.
```

I don't understand how it could pass for the cmake check yet fail later on? 
Are there multiple `libunwind.h` on freebsd, and it finds the wrong one here 
based on an unfortunate ordering of the include directories?

Or is there some other way for me to setup a freebsd environment without too 
much pain, so I can debug this issue directly?

Thanks

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

How to get dependencies into freebsd ci?

[Milian Wolff]

Hey all,

While looking at the kate CI setup, I saw that it gets build on freebsd. I 
would like to get that coverage too for heaptrack, but when I try to add

https://invent.kde.org/sysadmin/ci-utilities/raw/master/gitlab-templates/
freebsd.yml

Then heaptrack fails to find the elfutils dependency. This works on linux, but 
I have zero clue how that is done.

Can I add dependencies covered by `craft` to `.kde-ci.yml`? 

Where can I find documentation on what to put in there? Furthermore, is there 
some best practices when it comes to CI configuration for KDE projects? By 
chance I found https://community.kde.org/Infrastructure/GitLab/CI/
Static_Code_Analysis which is interesting - is there more like it somewhere?

Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: RFC: an improved ki18n_install

[Milian Wolff]

On Dienstag, 7. März 2023 02:51:06 CET Aleix Pol wrote:
> On Mon, Mar 6, 2023 at 8:31 PM Milian Wolff  wrote:
> > Hey all,
> > 
> > for context please see [1] and [2].
> > 
> > [1]: https://bugs.kde.org/show_bug.cgi?id=460245
> > [2]:
> > https://discourse.cmake.org/t/feature-request-add-custom-command-all/7609
> > 
> > The gist is that me and Igor are annoyed by `ki18n_install` abusing
> > `add_custom_target`, which makes the build always dirty. I.e. every time
> > we
> > run `ninja` we will, at the very least, run the two mo & ts generation
> > targets. For kdevelop, these do non-trivial amounts of work that takes
> > time
> > even on a beefy laptop:
> > 
> > ```
> > $ ninja && time ninja
> > [2/2] Generating mo...
> > [2/2] Generating mo...
> > 
> > real0m1,780s
> > user0m5,742s
> > sys 0m2,327s
> > ```
> > 
> > I would like to fix this, but first want to get feedback from the KDE
> > developers at large.
> > 
> > First of all: Are all projects using ki18n_install in the way we do? Why
> > is
> > no-one else complaining about this so far? Are your po files so small that
> > this doesn't take a significant amount of time? Or are there potentially
> > just so few of them in your project? KDevelop is heavily plugin based
> > which means we have tons of po files. 2464 *.po files to be exact...
> > 
> > Secondly: does anyone know how to best approach a solution to this issue?
> > The problem is that I don't see an easy way to solve it: While Kyle
> > Edwards was nice enough to show me a way to tell CMake to not make the
> > custom target always-dirty, `k18n_install` suffers from another design
> > deficiency: It uses globbing internally and has an undefined amount of
> > inputs and outputs, which basically makes it impossible for us to
> > leverage `add_custom_command(OUTPUT` here, or?
> > 
> > As such, it seems like one would need a different approach to this problem
> > anyways. Globbing is a known-evil in cmake land, and I would personally
> > prefer to have the inputs explicitly listed, just like we do for source
> > files etc. Because we have many languages though, listing all possible
> > *.po or *.ts files is cumbersome. Instead, what about we maintain the
> > list of known languages in the ki18n framework? Then we could have
> > something like
> > 
> > ```
> > ki18n_target(
> > 
> > # the target for which we are handling messages
> > TARGET KF5I18n
> > # the translation domain, added as compile_options and to find files
> > TRANSLATION_DOMAIN ki18n5
> > # the root po folder location, to look for the .po/.js files
> > PO_FOLDER ${KI18n_SOURCE_DIR}/po
> > 
> > )
> > ```
> > 
> > Internally, this would do what is currently done manually:
> > 
> > ```
> > target_compile_options(KF5I18n PRIVATE -DTRANSLATION_DOMAIN=\"ki18n5\")
> > ```
> > 
> > Then it would iterate over the list of known languages and try to find the
> > .po or .js file. For every match, we would add_custom_target to generate
> > the corresponding .mo/.ts file and add the reverse dependency.
> > 
> > What do others say to this approach?
> > 
> > Thanks
> > 
> > --
> > Milian Wolff
> > http://milianw.de
> 
> +1 to getting the problem solved. I've also been bothered by this and
> thought about looking into this so thanks for stepping up! (also I'm
> pretty sure this code is originally mine from a time when we didn't
> generate translations on every single build ^^').
> 
> Having ninja/make do this dependency generation can end up being more
> work than worth it because then we need to have a static list and then
> we need to re-run it when the po files change and it's all a mess.

The only mess that I can see is having to maintain the static list. Otherwise, 
we would just piggy back on the underlying buildsystem to drive the generation 
for us. Meaning, we would get separate targets for every .po/.js, leading to 
proper parallelization too. And only new/changed files would get re-generated 
as needed.

Anyhow, reaping those benefits is going to be hard due to the static list. As 
Albert said in his email, my proposal has no advantages over just using glob. 
As such, we have basically three options:

a) status quo:
 + trivial to setup
 + will always correctly track changes to the .po/.js files
 - serial instead of parallel generation of .mo/.ts files
 - always dirty ALL target

b) glob with separate targets:
 + trivial to setup
 + parallelized generation of .mo/.ts files

Re: RFC: an improved ki18n_install

[Milian Wolff]

On Dienstag, 7. März 2023 00:30:20 CET Albert Astals Cid wrote:
> El dilluns, 6 de març de 2023, a les 20:31:01 (CET), Milian Wolff va 
escriure:
> > Hey all,
> > 
> > for context please see [1] and [2].
> > 
> > [1]: https://bugs.kde.org/show_bug.cgi?id=460245
> > [2]:
> > https://discourse.cmake.org/t/feature-request-add-custom-command-all/7609
> > 
> > The gist is that me and Igor are annoyed by `ki18n_install` abusing
> > `add_custom_target`, which makes the build always dirty. I.e. every time
> > we
> > run `ninja` we will, at the very least, run the two mo & ts generation
> > targets. For kdevelop, these do non-trivial amounts of work that takes
> > time
> > even on a beefy laptop:
> > 
> > ```
> > $ ninja && time ninja
> > [2/2] Generating mo...
> > [2/2] Generating mo...
> > 
> > real0m1,780s
> > user0m5,742s
> > sys 0m2,327s
> > ```
> > 
> > I would like to fix this, but first want to get feedback from the KDE
> > developers at large.
> > 
> > First of all: Are all projects using ki18n_install in the way we do? Why
> > is
> > no-one else complaining about this so far? Are your po files so small that
> > this doesn't take a significant amount of time? Or are there potentially
> > just so few of them in your project? KDevelop is heavily plugin based
> > which
> > means we have tons of po files. 2464 *.po files to be exact...
> > 
> > Secondly: does anyone know how to best approach a solution to this issue?
> > The problem is that I don't see an easy way to solve it: While Kyle
> > Edwards
> > was nice enough to show me a way to tell CMake to not make the custom
> > target always-dirty, `k18n_install` suffers from another design
> > deficiency:
> > It uses globbing internally and has an undefined amount of inputs and
> > outputs, which basically makes it impossible for us to leverage
> > `add_custom_command(OUTPUT` here, or?
> > 
> > As such, it seems like one would need a different approach to this problem
> > anyways. Globbing is a known-evil in cmake land, and I would personally
> > prefer to have the inputs explicitly listed, just like we do for source
> > files etc. Because we have many languages though, listing all possible
> > *.po
> > or *.ts files is cumbersome. Instead, what about we maintain the list of
> > known languages in the ki18n framework? Then we could have something like
> > 
> > ```
> > ki18n_target(
> > 
> > # the target for which we are handling messages
> > TARGET KF5I18n
> > # the translation domain, added as compile_options and to find files
> > TRANSLATION_DOMAIN ki18n5
> > # the root po folder location, to look for the .po/.js files
> > PO_FOLDER ${KI18n_SOURCE_DIR}/po
> > 
> > )
> > ```
> > 
> > Internally, this would do what is currently done manually:
> > 
> > ```
> > target_compile_options(KF5I18n PRIVATE -DTRANSLATION_DOMAIN=\"ki18n5\")
> > ```
> > 
> > Then it would iterate over the list of known languages and try to find the
> > .po or .js file. For every match, we would add_custom_target to generate
> > the corresponding .mo/.ts file and add the reverse dependency.
> > 
> > What do others say to this approach?
> 
> How is globbing (checking what is in the filesystem) different from the
> checking against a known list that will check against the filesystem if a
> file exists?
> 
> Seems the same thing but with extra steps to me.

You are totally right, it really is the same thing now that I think about this 
again. The fundamental issue is finding a solution where we know the list of 
inputs/outputs without having to rerun cmake. This is not possible without a 
predefined list of inputs, which is very cumbersome.

So that means we have to live with the always-dirty all target? That seems to 
be very unfortunate for me, but I don't have a better solution either :( 
Thankfully Aleix's patch should make the pain less pronounced at least

Thanks

-- 
Milian Wolff
http://milianw.de

RFC: an improved ki18n_install

[Milian Wolff]

Hey all,

for context please see [1] and [2].

[1]: https://bugs.kde.org/show_bug.cgi?id=460245
[2]: https://discourse.cmake.org/t/feature-request-add-custom-command-all/7609

The gist is that me and Igor are annoyed by `ki18n_install` abusing 
`add_custom_target`, which makes the build always dirty. I.e. every time we 
run `ninja` we will, at the very least, run the two mo & ts generation 
targets. For kdevelop, these do non-trivial amounts of work that takes time 
even on a beefy laptop:

```
$ ninja && time ninja
[2/2] Generating mo...
[2/2] Generating mo...

real0m1,780s
user0m5,742s
sys 0m2,327s
```

I would like to fix this, but first want to get feedback from the KDE 
developers at large.

First of all: Are all projects using ki18n_install in the way we do? Why is 
no-one else complaining about this so far? Are your po files so small that 
this doesn't take a significant amount of time? Or are there potentially just 
so few of them in your project? KDevelop is heavily plugin based which means 
we have tons of po files. 2464 *.po files to be exact...

Secondly: does anyone know how to best approach a solution to this issue? The 
problem is that I don't see an easy way to solve it: While Kyle Edwards was 
nice enough to show me a way to tell CMake to not make the custom target 
always-dirty, `k18n_install` suffers from another design deficiency: It uses 
globbing internally and has an undefined amount of inputs and outputs, which 
basically makes it impossible for us to leverage `add_custom_command(OUTPUT` 
here, or?

As such, it seems like one would need a different approach to this problem 
anyways. Globbing is a known-evil in cmake land, and I would personally prefer 
to have the inputs explicitly listed, just like we do for source files etc.
Because we have many languages though, listing all possible *.po or *.ts files 
is cumbersome. Instead, what about we maintain the list of known languages in 
the ki18n framework? Then we could have something like 

```
ki18n_target(
# the target for which we are handling messages
TARGET KF5I18n
# the translation domain, added as compile_options and to find files
TRANSLATION_DOMAIN ki18n5
# the root po folder location, to look for the .po/.js files
PO_FOLDER ${KI18n_SOURCE_DIR}/po
)
```

Internally, this would do what is currently done manually:

```
target_compile_options(KF5I18n PRIVATE -DTRANSLATION_DOMAIN=\"ki18n5\")
```

Then it would iterate over the list of known languages and try to find the .po 
or .js file. For every match, we would add_custom_target to generate the 
corresponding .mo/.ts file and add the reverse dependency.

What do others say to this approach?

Thanks

-- 
Milian Wolff
http://milianw.de

Re: New santizer warning in KF 5.98 headers

[Milian Wolff]

On Mittwoch, 11. Januar 2023 11:26:42 CET Ingo Klöcker wrote:
> On Mittwoch, 11. Januar 2023 11:02:04 CET Milian Wolff wrote:
> > On Dienstag, 10. Januar 2023 23:45:26 CET Michael Reeves wrote:
> > > Thanks. I would say your right there this would definitely have caught
> > > someone's attention if didn't work in practice with what kde needs.
> > > Santizers are by design quite pedantic as serves there purpose well.
> > 
> > I agree, the code is clearly wrong and it's unclear what it's trying to
> > achieve here. Does anyone know what this is trying to do?
> > 
> > Qt::ConnectionType connectionType = static_cast(-1)
> > 
> > Should this maybe just be changed to use Qt::AutoConnection?
> 
> The code:
> https://invent.kde.org/frameworks/kconfigwidgets/-/blob/master/src/
> kstandardaction.h#L253
> 
> What the code does:
> The default value `static_cast(-1)` serves as hint that
> the function should decide what kind of connection type to use and for some
> reason the `ConfigureToolbars` action explicitly needs to use a
> `Qt::QueuedConnection` instead of a `Qt::AutoConnection`. This could be
> changed to a std::optional (for KF6) to make the intention clear.

Indeed, I should have read the body of the function and not stop at the 
signature only :)

Thanks for the explanation Ingo! And std::optional for KF6 would be great imo, 
if this is really needed.

> Why the code does what it does:
> One could question whether this special casing for `ConfigureToolbars` is
> still necessary. The bug report about the crash that this seems to have
> fixed is from 2009: https://bugs.kde.org/show_bug.cgi?id=200815
> 
> Regards,
> Ingo


-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: New santizer warning in KF 5.98 headers

[Milian Wolff]

On Dienstag, 10. Januar 2023 23:45:26 CET Michael Reeves wrote:
> Thanks. I would say your right there this would definitely have caught
> someone's attention if didn't work in practice with what kde needs.
> Santizers are by design quite pedantic as serves there purpose well.

I agree, the code is clearly wrong and it's unclear what it's trying to 
achieve here. Does anyone know what this is trying to do?

Qt::ConnectionType connectionType = static_cast(-1)

Should this maybe just be changed to use Qt::AutoConnection?

Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: Loosening the commit limit for work branches

[Milian Wolff]

On Mittwoch, 24. August 2022 18:57:12 CEST Noah Davis wrote:
> I didn't want to leave the master branch in a semi-broken state or
> present what I had come up with in an MR until I could present it as
> an improvement. It took a while to figure out what I wanted and
> whether some of what I wanted was even achievable. I haven't been
> working alone, so some people such as Marco and Nate are aware of the
> current state of the branch and have been testing it periodically. I'm
> aiming for a 22.12 release, so I intend to finish the patch over the
> next month (it's already getting close), leaving 2 or 3 months for
> additional testing after the merge.

I see, these are very valid reasons. Eventually though the situation should 
settle down, at which point some squashing is going to be required anyways to 
ensure that the commit history stays atomic. I.e. no single commit should 
yield a semi-broken state, as that would totally destroy `git bisect` and 
similar workflows. Anyhow, enough of this, as it's really not the point of 
this thread as you are writing below :)

> I don't want this discussion to turn into whether or not only my
> reasons for wanting a higher commit limit/no limit are right, but I
> recognize it is important to question my reasons since those are a
> factor in the discussion.

I totally agree with that. And just to make it clear again: I'm only trying to 
shine a light on how one could potentially handle such discussions 
differently. In the end it's you who does the work, and that counts infinitely 
more so don't let me deter you from your goals here! And as I said previously, 
I'm totally in favor of raising the commit limit, if possible.

Cheers

-- 
Milian Wolff
http://milianw.de

Re: Loosening the commit limit for work branches

[Milian Wolff]

On Mittwoch, 24. August 2022 17:26:33 CEST Noah Davis wrote:
> On Wed, Aug 24, 2022 at 5:12 PM Milian Wolff  wrote:
> > Without any knowledge of your work on the QML port of Spectacle, I would
> > also claim that there is bound to be a lot of generic work that should be
> > possible to land directly in the main branch, no? You are probably
> > splitting up the data representation and UI layer, which can happen
> > early. Then you add a second UI implementation in tandem to the widget
> > one, which can be opt-in until it's ready. Once all is done, you can
> > remove the old widget UI. There's no need to wait a long time for all of
> > this to hit the main branch and work only in a feature branch, no?
> 
> The UI is already fairly well separated from the backend simply
> because Spectacle already has a CLI. I simply went through a lot of
> iterations over the past few months in collaboration with Marco while
> trying to come up with the right UI/UX. The branch contains a lot of
> new UI related code that uses Qt Quick/QML. It would be useful for me
> to keep track of these changes so that if anything breaks in the
> process, I can more easily figure out which change did it and ask
> questions if I wasn't the one who made the change.

Right, as I said I only worked on assumptions in my statement above.

What prevents you though from merging the partial state of the Qt Quick/QML UI 
into the main branch? If you say the code history as it is now is useful, it 
should be useful in the future too - and as such could be merged more 
regularly? You don't have to enable the new UI for now in the main branch?

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: Loosening the commit limit for work branches

[Milian Wolff]

On Mittwoch, 24. August 2022 15:30:06 CEST Noah Davis wrote:
> A week ago, I ran into an unexpected issue while working on a QML port
> of Spectacle. There is an undocumented pre-receive hook that sets a
> 100 commit limit for all branches of official repos, including work
> branches. The error message it gave me told me to file a sysadmin
> ticket, so I did that.



> What does the broader KDE development community think? Should there be
> a commit limit and how large should it be? Only KDE devs can make work
> branches, so the pool of people who can make branches with large
> amounts of commits is already fairly limited.

I have also run into this but got done at around ~90ish patches and was 
notified by Nicolas in time about this limitation.

Generally, such arbitrary limitations are always a bad sign imo and we should 
ideally try to remove or increase the limit. Lots of small patches are far 
easier to work with than few mega patches.

That said, from my personal knowledge it's usually a bad sign to start mega 
rewrites or refactors in a branch - sooner or later you should hit a small 
mile stone that you can merge already. If you don't do that and pile up tons 
of patches in your work branch, you might get into rebase or merge conflict 
hell when the main branch continues to see development. I've been there and 
just started over, wasting quite a bit of time. Since then, I'm trying to look 
for small work packages that can be hoisted out of the larger work branches 
and merged early. This then allows to reduce the size of the work branch, 
without having to use large squashed mega patches.

Without any knowledge of your work on the QML port of Spectacle, I would also 
claim that there is bound to be a lot of generic work that should be possible 
to land directly in the main branch, no? You are probably splitting up the 
data representation and UI layer, which can happen early. Then you add a 
second UI implementation in tandem to the widget one, which can be opt-in 
until it's ready. Once all is done, you can remove the old widget UI. There's 
no need to wait a long time for all of this to hit the main branch and work 
only in a feature branch, no?

Cheers
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: Approval request for feature idea

[Milian Wolff]

On Montag, 30. Mai 2022 02:33:56 CEST samuel ammonius wrote:
> I wanted to submit a feature for kde that would allow users to set the
> application style to a qstylesheet file (style.qss for example). This
> feature would greatly increase the amount of themes available for KDE, make
> it possible for users to tweak themes themselves, and make it easier to
> include custom icons/images with themes if necessary by using the url()
> function in the stylesheet. It would also make it easier for experienced
> developers who can make KDE themes in C++ to make multiple, more complex
> themes in less time. The feature would also be very easy to make and would
> probably take less than 1 Megabyte of total storage.
> 
> I originally wanted to submit this as a project in Google summer of code,
> but I missed the deadline for applying. Who should I contact to get
> approval for this before submitting a pull request?

https://www.kdab.com/say-no-to-qt-style-sheets/

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: Difference between QConcatenateTablesProxyModel and KConcatenateRowsProxyModel

[Milian Wolff]

On Friday, April 22, 2022 3:39:08 AM CEST Fusion Future wrote:
> One major difference I have found is that KConcatenateRowsProxyModel
> will check if sourceModel exists when mapping from/to sourceModel, but
> QConcatenateTablesProxyModel won't. So if QConcatenateTablesProxyModel
> doesn't receive `rowsRemoved` signal from sourceModel,
> QConcatenateTablesProxyModel will crash the program.

As an outsider, I have to say that this sounds perfectly fine - even desired. 
Not following the QAIM protocols pedantically will have all sorts of 
unintended issues otherwise, some of which are hard to spot and test for. I 
can only recommend to test every model with the QAbstractItemModelTester, 
which will probably also complain about the above situation from what I 
understand.

Cheers

-- 
Milian Wolff | milian.wo...@kdab.com | Senior Software Engineer
KDAB (Deutschland) GmbH, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt, C++ and OpenGL Experts

smime.p7s
Description: S/MIME cryptographic signature

Re: State of server-side retrace of KDE crashes?

[Milian Wolff]

On Donnerstag, 16. Dezember 2021 23:27:26 CET Lyubomir Parvanov wrote:
> On Arch the situation with the debug symbols is tragic for the bad, we get
> the first bugs (and the first fixes), but we can't report bugs that are
> hard to reproduce because we have to reinstall the whole goddamn apps for
> symbols to work and then to pray to God to be able to reproduce the bug
> again.
> There were some talks about adding a server-side retrace service for KDE,
> what's the latest status quo? I really believe that KDE would be way more
> stable if this was existing...

Ideally such a service would be based on debuginfod from elfutils, but it 
needs to be integrated on the package builder side. I.e. this would need to be 
done by the Arch packagers and it nothing that can be influenced by KDE itself 
directly. The reason is that the debug symbols have to match exactly the 
binary that is run.

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: Noteworthy changes when porting to C++17

[Milian Wolff]

On Sonntag, 18. Juli 2021 02:34:24 CEST Frederik Schwarzer wrote:
> Hi,
> 
> since we are increasing the C++ standard requirement from 11 to 17 with
> KF6 and there were a few deprecations/removals in between, I wonder if
> any of those are noteworthy for people developing applications based on
> KDE Frameworks.
> 
> What I mean by "noteworthy" is features that are commonly used or at
> least known to be used sometimes in our ecosystem. Things like the
> "register" keyword for example might not be found in high-level
> applications so pointing KDE developers to its removal might get you
> shrugs in return.
> 
> What I have seen is that std::mem_fun was used within KIO and has been
> replaced by std::mem_fn. Not sure if that counts as "commonly used", though.

Imo, usages of either of these two should be rewritten to use lambdas instead.

> Compiler vendors seem to be handling those removalss differently. The
> libstdc++ devs have not had deprecation warnings for at least some of
> the stuff that was deprecated in C++11, so they will not remove those
> any time soon. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91383#c1
> 
> In libc++, the deprecation warnings were shown since C++11 and now with
> C++17 they removed some stuff. On Linux you will have to build with the
> -stdlib=libc++ option for clang to notice. See e.g.
> https://godbolt.org/z/6Y1eE3z4P for playing with it.
> 
> But I digress ...
> 
> So the question is: did you notice things that have been removed from
> the C++ standard since C++11 that were used in our applications?


-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: Notice of withdrawal of CI services: KDevelop and KDE Connect

[Milian Wolff]

On Donnerstag, 17. Juni 2021 11:25:15 CEST Ben Cooksley wrote:
> On Thu, Jun 17, 2021 at 8:44 AM Milian Wolff  wrote:
> > On Mittwoch, 16. Juni 2021 20:28:25 CEST Ben Cooksley wrote:
> > > Hi all,
> > > 
> > > 
> > > The following is notice that I intend to withdraw CI services from the
> > > following two KDE projects due to faults in their code or build system
> > > which are having a significant adverse impact on the CI system and
> > > negatively impacting on other projects:
> > > 
> > > - KDevelop
> > > - KDE Connect
> > > 
> > > This withdrawal will be applied to all platforms.
> > > 
> > > In the case of KDevelop, it has a series of unit tests which on FreeBSD
> > > cause gdb to hang and consume an entire CPU core indefinitely. This
> > > slows
> > > down builds for all other projects using that CI server, and also
> > 
> > prevents
> > 
> > > KWin tests from proceeding - completely blocking it's jobs. This fault
> > > is
> > > in the debuggee_slow family of tests.
> > 
> > Hey Ben,
> 
> Hi Milian,
> 
> > first time I hear of this issue. I simply don't have the capacity to track
> > kdevelop CI mails, so if anything really bad happens, I would really
> > appreciate if anyone who notices that sents a mail to the kdevelop mailing
> > list so we can act on it. I still use kdevelop daily, but don't put any
> > other
> > work in it currently due to lack of time...
> 
> The issue in question was noted on #kdevelop (prior to the whole Freenode
> meltdown) on a few occasions.

Even before that meltdown, I only looked at this when I was directly pinged. 
Please prefer to use the mailing lists to get our attention.

> > That said: can we just disable it on the FreeBSD platform, if that's the
> > only
> > one that exhibits this failure? Alternatively I'm obviously fine with
> > skipping
> > that test on that platform, can you give me a link to a failure please so
> > I
> > can see which tests exactly are affected? Then I'll add the QSKIP +
> > platform
> > ifdef checks to skip it on freebsd.
> 
> I'm afraid I only have records of the hung test processes (and the gdb
> processes above them using an entire CPU core indefinitely each).
> 
> jenkins60635   0.0  0.0   12120   2256  0  TXs+ 02:44 0:00.00
> /usr/home/jenkins/workspace/KDevelop/kdevelop/kf5-qt5
> FreeBSDQt5.15/build/plugins/debuggercommon/tests/debuggees/debuggee_debugees
> low (debuggee_debugeeslo)
> jenkins74167   0.0  0.0   12120   2260  1  TXs+ 02:55 0:00.00
> /usr/home/jenkins/workspace/KDevelop/kdevelop/kf5-qt5
> FreeBSDQt5.15/build/plugins/debuggercommon/tests/debuggees/debuggee_debugees
> low (debuggee_debugeeslo)
> 
> Hopefully that is enough to identify the tests that are broken?

I have now added code to skip the tests on FreeBSD wherever debuggeeslow is 
used. Unsure which test of the many is actually triggering it, but I have no 
time to invest on this either.

So, is that acceptable for you for now?

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: Notice of withdrawal of CI services: KDevelop and KDE Connect

[Milian Wolff]

On Donnerstag, 17. Juni 2021 11:25:15 CEST Ben Cooksley wrote:
> On Thu, Jun 17, 2021 at 8:44 AM Milian Wolff  wrote:
> > On Mittwoch, 16. Juni 2021 20:28:25 CEST Ben Cooksley wrote:
> > > Hi all,
> > > 
> > > 
> > > The following is notice that I intend to withdraw CI services from the
> > > following two KDE projects due to faults in their code or build system
> > > which are having a significant adverse impact on the CI system and
> > > negatively impacting on other projects:
> > > 
> > > - KDevelop
> > > - KDE Connect
> > > 
> > > This withdrawal will be applied to all platforms.
> > > 
> > > In the case of KDevelop, it has a series of unit tests which on FreeBSD
> > > cause gdb to hang and consume an entire CPU core indefinitely. This
> > > slows
> > > down builds for all other projects using that CI server, and also
> > 
> > prevents
> > 
> > > KWin tests from proceeding - completely blocking it's jobs. This fault
> > > is
> > > in the debuggee_slow family of tests.
> > 
> > Hey Ben,
> 
> Hi Milian,
> 
> > first time I hear of this issue. I simply don't have the capacity to track
> > kdevelop CI mails, so if anything really bad happens, I would really
> > appreciate if anyone who notices that sents a mail to the kdevelop mailing
> > list so we can act on it. I still use kdevelop daily, but don't put any
> > other
> > work in it currently due to lack of time...
> 
> The issue in question was noted on #kdevelop (prior to the whole Freenode
> meltdown) on a few occasions.

Even before that meltdown, I only looked at this when I was directly pinged. 
Please prefer to use the mailing lists to get our attention.

> > That said: can we just disable it on the FreeBSD platform, if that's the
> > only
> > one that exhibits this failure? Alternatively I'm obviously fine with
> > skipping
> > that test on that platform, can you give me a link to a failure please so
> > I
> > can see which tests exactly are affected? Then I'll add the QSKIP +
> > platform
> > ifdef checks to skip it on freebsd.
> 
> I'm afraid I only have records of the hung test processes (and the gdb
> processes above them using an entire CPU core indefinitely each).
> 
> jenkins60635   0.0  0.0   12120   2256  0  TXs+ 02:44 0:00.00
> /usr/home/jenkins/workspace/KDevelop/kdevelop/kf5-qt5
> FreeBSDQt5.15/build/plugins/debuggercommon/tests/debuggees/debuggee_debugees
> low (debuggee_debugeeslo)
> jenkins74167   0.0  0.0   12120   2260  1  TXs+ 02:55 0:00.00
> /usr/home/jenkins/workspace/KDevelop/kdevelop/kf5-qt5
> FreeBSDQt5.15/build/plugins/debuggercommon/tests/debuggees/debuggee_debugees
> low (debuggee_debugeeslo)
> 
> Hopefully that is enough to identify the tests that are broken?

I have now added code to skip the tests on FreeBSD wherever debuggeeslow is 
used. Unsure which test of the many is actually triggering it, but I have no 
time to invest on this either.

So, is that acceptable for you for now?

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: Notice of withdrawal of CI services: KDevelop and KDE Connect

[Milian Wolff]

On Mittwoch, 16. Juni 2021 20:28:25 CEST Ben Cooksley wrote:
> Hi all,
> 
> The following is notice that I intend to withdraw CI services from the
> following two KDE projects due to faults in their code or build system
> which are having a significant adverse impact on the CI system and
> negatively impacting on other projects:
> 
> - KDevelop
> - KDE Connect
> 
> This withdrawal will be applied to all platforms.
> 
> In the case of KDevelop, it has a series of unit tests which on FreeBSD
> cause gdb to hang and consume an entire CPU core indefinitely. This slows
> down builds for all other projects using that CI server, and also prevents
> KWin tests from proceeding - completely blocking it's jobs. This fault is
> in the debuggee_slow family of tests.

Hey Ben,

first time I hear of this issue. I simply don't have the capacity to track 
kdevelop CI mails, so if anything really bad happens, I would really 
appreciate if anyone who notices that sents a mail to the kdevelop mailing 
list so we can act on it. I still use kdevelop daily, but don't put any other 
work in it currently due to lack of time...

That said: can we just disable it on the FreeBSD platform, if that's the only 
one that exhibits this failure? Alternatively I'm obviously fine with skipping 
that test on that platform, can you give me a link to a failure please so I 
can see which tests exactly are affected? Then I'll add the QSKIP + platform 
ifdef checks to skip it on freebsd.

Cheers and thanks for letting me know about this problem

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: Notice of withdrawal of CI services: KDevelop and KDE Connect

[Milian Wolff]

On Mittwoch, 16. Juni 2021 20:28:25 CEST Ben Cooksley wrote:
> Hi all,
> 
> The following is notice that I intend to withdraw CI services from the
> following two KDE projects due to faults in their code or build system
> which are having a significant adverse impact on the CI system and
> negatively impacting on other projects:
> 
> - KDevelop
> - KDE Connect
> 
> This withdrawal will be applied to all platforms.
> 
> In the case of KDevelop, it has a series of unit tests which on FreeBSD
> cause gdb to hang and consume an entire CPU core indefinitely. This slows
> down builds for all other projects using that CI server, and also prevents
> KWin tests from proceeding - completely blocking it's jobs. This fault is
> in the debuggee_slow family of tests.

Hey Ben,

first time I hear of this issue. I simply don't have the capacity to track 
kdevelop CI mails, so if anything really bad happens, I would really 
appreciate if anyone who notices that sents a mail to the kdevelop mailing 
list so we can act on it. I still use kdevelop daily, but don't put any other 
work in it currently due to lack of time...

That said: can we just disable it on the FreeBSD platform, if that's the only 
one that exhibits this failure? Alternatively I'm obviously fine with skipping 
that test on that platform, can you give me a link to a failure please so I 
can see which tests exactly are affected? Then I'll add the QSKIP + platform 
ifdef checks to skip it on freebsd.

Cheers and thanks for letting me know about this problem

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: Looking for an SVGDOM / other SVG library for use with C++ and CMake.

[Milian Wolff]

On Donnerstag, 10. Juni 2021 23:45:20 CEST David Hurka wrote:
> Yes, I am about to write a minimal SVG parser with QDomDocument. The result
> is probably very little code. I would just like to outsource this task to
> someone who already implemented and tested it. :)

Don't use QDomDocument, Qt XML is deprecated. Use QXmlStreamReader instead.

> breeze-icon-cleaner splits one SVG drawing into multiple icon files, where
> the file names are passed as command line option. I think it would be more
> convenient to define the file names directly in the SVG drawing.
> 
> So I am looking for a library that gives me access to text elements in an
> SVG
> file. It should be usable from C++ and with CMake.

This sounds like a trivial 1to1 filter operation that just adds an attribute 
at a certain point. Doing that with QXmlStreamReader / Writer should be pretty 
straight forward.

Alternatively, why not write a simple script for this using Python e.g.? You 
could even use XSLT for this purpose. I don't quite see what you need C++ for 
here, personally.

Cheers
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: New repo in kdereview: kalk

[Milian Wolff]

On Donnerstag, 18. Februar 2021 23:55:32 CET Albert Astals Cid wrote:
> El dijous, 18 de febrer de 2021, a les 17:05:22 CET, hanyoung va escriure:
> > Hello everyone!
> > 
> > I want to move kalk to kdereview.
> > It's the calculator for Plasma Mobile (it also works great on desktop).
> > 
> > https://invent.kde.org/plasma-mobile/kalk
> > 
> > It's not a QML version of kcalc, the math engine is written with
> > bison/flex.
> I don't think you should roll out your own math engine.
> 
> Example, if I press
> 65 / 9 =
> * 9 =
> it tells me the result is 63.
> 
> Compare to kcalc that properly tells me 65.
> 
> You can't really use C/C++ floating point to do actual math, it's not very
> good for that, you need to use "infinite" precision numbers like kcalc
> does.
> 
> Please reconsider your decision.

https://blog.acolyer.org/2020/10/02/toward-an-api-for-the-real-numbers/

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: Retiring

[Milian Wolff]

On Mittwoch, 13. Januar 2021 21:57:13 CET Christoph Feck wrote:
> Hello developers,
> 
> my personal situation allows for much less time for KDE
> related work, at least during the Corona times.
> 
> I would like to retire as soon as possible from these
> responsibilities:
> - bug triaging
> - release service
> - KIconTheme maintainer
> - KPlotting maintainer
> - KWidgetAddons maintainer
> 
> For bugzilla, I see many new and old contributors doing awesome
> work with triaging, so departing here shouldn't be a big issue.
> I hope someone can continue checking responses to NEEDINFO bugs
> and REPORTED bugs that didn't get a reply within about two weeks.
> 
> Regarding the release service work, I have made sure the load
> isn't all back to Albert. For future release work, Heiko Becker
> volunteered to help, but maybe another hand would be awesome, too.
> 
> For the frameworks modules, I initially assumed each module
> must have a separate maintainer, so I volunteered. Today I
> see many modules still just community-maintained, and I hope
> the community can take over maintainance. Not that I did any
> relevant work here in the recent years...
> 
> I might eventually continue to prepare some patches, so please
> keep all my accounts intact, but only de-list me as the maintainer.

Thanks a lot Christoph for all your work! As I myself has had the pleasure of 
quasi-retiring but never quite leaving - may the same happen to you ;-) I.e.: 
Take your well earned break and come back whenever it suites you again!

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: Failing builds on the CI system

[Milian Wolff]

On Samstag, 28. November 2020 03:42:21 CET Ben Cooksley wrote:
> Hi all,
> 
> Yesterday evening a number of projects had their jobs on the CI system
> regenerated, as part of Frameworks moving off Qt 5.12 and on to a minimum
> build of Qt 5.14.
> 
> In keeping with prior precedence, this means that Applications and all
> other builds then move on to the currently supported mainstream version of
> Qt, which at the moment is Qt 5.15.
> 
> While the Dependency Builds did mostly complete without issue, as part of
> this a number of long running failures were noted, which it would be nice
> if people could please correct. In no particular order:



> - KDevelop's Python Support: Appears to be broken due to SIC changes in
> KDevelop itself
> - KDevelop XDebug Support: Same as the Python support

Both fixed now, sorry for the delay.

> - KDevelop: Appears to be performing an operation that is not permitted on
> Windows in public interfaces

We are on it, any help is welcome. See:

https://invent.kde.org/kdevelop/kdevelop/-/merge_requests/181#note_136733
https://invent.kde.org/kdevelop/kdevelop/-/merge_requests/194#note_136805

We will probably de-inline the corresponding functions in Windows to unbreak 
this for now...

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: CMake kauth_install_* with system kauth and user CMAKE_INSTALL_PREFIX

[Milian Wolff]

On Montag, 19. Oktober 2020 17:21:28 CEST Harald Sitter wrote:
> On 19.10.20 17:06, Milian Wolff wrote:
> > On Montag, 19. Oktober 2020 16:56:27 CEST Harald Sitter wrote:
> >> On 19.10.20 12:30, Milian Wolff wrote:
> >>> On Montag, 19. Oktober 2020 11:27:29 CEST Harald Sitter wrote:
> >>>> Yo
> >>>> 
> >>>> On 18.10.20 10:11, Milian Wolff wrote:
> >>>>> Hey all,
> >>>>> 
> >>>>> since some time now I'm only compiling parts of KF5 selectively into a
> >>>>> custom prefix. Most notably, I'm only interested in ktexteditor and
> >>>>> syntax- highlighting, and would like to obtain all other frameworks
> >>>>> through my distribution packages.
> >>>>> 
> >>>>> Sadly, this doesn't work as easily, as ktexteditor is trying to do
> >>>>> this:
> >>>>> 
> >>>>> ```
> >>>>> install(TARGETS kauth_ktexteditor_helper DESTINATION $
> >>>>> {KAUTH_HELPER_INSTALL_DIR} )
> >>>>> kauth_install_helper_files(kauth_ktexteditor_helper
> >>>>> org.kde.ktexteditor.katetextbuffer root)
> >>>>> kauth_install_actions(org.kde.ktexteditor.katetextbuffer buffer/
> >>>>> org.kde.ktexteditor.katetextbuffer.actions)
> >>>>> ```
> >>>>> 
> >>>>> Because my KAuth is a system-provided installation, and KTextEditor
> >>>>> should
> >>>>> be installed into a user-writable prefix, I get this error on `ninja
> >>>>> install`:
> >>>>> 
> >>>>> ```
> >>>>> 
> >>>>> CMake Error at src/cmake_install.cmake:143 (file):
> >>>>>   file INSTALL cannot copy file
> >>>>>   "/home/milian/projects/kf5/build/frameworks/ktexteditor/src/
> >>>>> 
> >>>>> org.kde.ktexteditor.katetextbuffer.policy"
> >>>>> 
> >>>>>   to
> >>>>>   "/usr/share/polkit-1/actions/org.kde.ktexteditor.katetextbuffer.poli
> >>>>>   cy
> >>>>>   "
> >>>>>   
> >>>>>   Permission denied.
> >>>>> 
> >>>>> Call Stack (most recent call first):
> >>>>>   cmake_install.cmake:77 (include)
> >>>>> 
> >>>>> ```
> >>>>> 
> >>>>> I can workaround this by either commenting out the kauth install bits
> >>>>> in
> >>>>> ktexteditor or by installing kauth seperately too. But both are in my
> >>>>> opinion not ideal.
> >>>>> 
> >>>>> I don't think it's currently possible to overwrite the KAuth install
> >>>>> directory at cmake configure time. I would like to change that, unless
> >>>>> there is something I'm missing which would prevent this? I am hoping
> >>>>> that
> >>>>> any folder works as long as it's listed in the `XDG_DATA_DIRS` env var
> >>>>> -
> >>>>> can someone confirm this?
> >>>> 
> >>>> Alas, the path is hardcoded at buildtime from what I see:
> >>>> https://gitlab.freedesktop.org/polkit/polkit/-/blob/master/src/polkitba
> >>>> ck
> >>>> end /polkitbackendinteractiveauthority.c#L302
> >>>> 
> >>>> Relevant: https://bugs.kde.org/show_bug.cgi?id=425272
> >>> 
> >>> So the path that needs to be used is actually the polkit path?
> >> 
> >> That's my understanding of the code, yes.
> >> 
> >>> Meaning the
> >>> question where KAuth is being installed to doesn't really matter? That
> >>> would imply that KAuth is broken anyways when it's not installed to the
> >>> same prefix that polkit is being installed to, no?
> >> 
> >> Nope. If the policy was installed to a different path it would be
> >> broken, but the very error you posted is because kauth is insisting on
> >> putting the policy in the correct path rather than the prefix the rest
> >> of the build is installing to.
> > 
> > No, it's using the install location of kauth, it doesn't query polkit
> > itself. I.e. if I hand-compile KAuth and install into a custom prefix,
> > then I will have:
> > 
> > ```
> > $ grep INSTALL /home/milian/projects/compiled/kf5-dbg/lib/c

Re: CMake kauth_install_* with system kauth and user CMAKE_INSTALL_PREFIX

[Milian Wolff]

On Montag, 19. Oktober 2020 13:33:38 CEST Christophe Giboudeaux wrote:
> On lundi 19 octobre 2020 12:30:59 CEST Milian Wolff wrote:
> > On Montag, 19. Oktober 2020 11:27:29 CEST Harald Sitter wrote:
> > > Yo
> > > 
> > > On 18.10.20 10:11, Milian Wolff wrote:
> > > > Hey all,
> > > > 
> > > > since some time now I'm only compiling parts of KF5 selectively into a
> > > > custom prefix. Most notably, I'm only interested in ktexteditor and
> > > > syntax- highlighting, and would like to obtain all other frameworks
> > > > through my distribution packages.
> > > > 
> > > > Sadly, this doesn't work as easily, as ktexteditor is trying to do
> > > > this:
> > > > 
> > > > ```
> > > > install(TARGETS kauth_ktexteditor_helper DESTINATION $
> > > > {KAUTH_HELPER_INSTALL_DIR} )
> > > > kauth_install_helper_files(kauth_ktexteditor_helper
> > > > org.kde.ktexteditor.katetextbuffer root)
> > > > kauth_install_actions(org.kde.ktexteditor.katetextbuffer buffer/
> > > > org.kde.ktexteditor.katetextbuffer.actions)
> > > > ```
> > > > 
> > > > Because my KAuth is a system-provided installation, and KTextEditor
> > > > should
> > > > be installed into a user-writable prefix, I get this error on `ninja
> > > > install`:
> > > > 
> > > > ```
> > > > 
> > > > CMake Error at src/cmake_install.cmake:143 (file):
> > > >   file INSTALL cannot copy file
> > > >   "/home/milian/projects/kf5/build/frameworks/ktexteditor/src/
> > > > 
> > > > org.kde.ktexteditor.katetextbuffer.policy"
> > > > 
> > > >   to
> > > >   "/usr/share/polkit-1/actions/org.kde.ktexteditor.katetextbuffer.poli
> > > >   cy
> > > >   "
> > > >   
> > > >   Permission denied.
> > > > 
> > > > Call Stack (most recent call first):
> > > >   cmake_install.cmake:77 (include)
> > > > 
> > > > ```
> > > > 
> > > > I can workaround this by either commenting out the kauth install bits
> > > > in
> > > > ktexteditor or by installing kauth seperately too. But both are in my
> > > > opinion not ideal.
> > > > 
> > > > I don't think it's currently possible to overwrite the KAuth install
> > > > directory at cmake configure time. I would like to change that, unless
> > > > there is something I'm missing which would prevent this? I am hoping
> > > > that
> > > > any folder works as long as it's listed in the `XDG_DATA_DIRS` env var
> > > > -
> > > > can someone confirm this?
> > > 
> > > Alas, the path is hardcoded at buildtime from what I see:
> > > https://gitlab.freedesktop.org/polkit/polkit/-/blob/master/src/polkitbac
> > > ke
> > > nd /polkitbackendinteractiveauthority.c#L302
> > > 
> > > Relevant: https://bugs.kde.org/show_bug.cgi?id=425272
> > 
> > So the path that needs to be used is actually the polkit path? Meaning the
> > question where KAuth is being installed to doesn't really matter? That
> > would imply that KAuth is broken anyways when it's not installed to the
> > same prefix that polkit is being installed to, no?
> 
> Yes. Polkit is a security tool, it doesn't allow reading files in random
> locations.

Sure, I understand this reasoning. But I don't get how one is supposed to use 
it then as a developer - I don't want to overwrite the polkit files that got 
installed through my package manager e.g.

> > If so, then it's just one more reason to be able to skip kauth policy
> > installation, as they won't be picked up anyways...
> 
> kauth can be configured with -DKAUTH_BACKEND_NAME=FAKE

I don't want to build kauth, I want to build ktexteditor, so this is where I'd 
like to disable the polkit installation. But ideally that should be generic 
and applicable to all users of KAuth - I'll prepare a patch for KAuth to get a 
cmake cache var added that can be used for this purpose then.

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: CMake kauth_install_* with system kauth and user CMAKE_INSTALL_PREFIX

[Milian Wolff]

On Montag, 19. Oktober 2020 16:56:27 CEST Harald Sitter wrote:
> On 19.10.20 12:30, Milian Wolff wrote:
> > On Montag, 19. Oktober 2020 11:27:29 CEST Harald Sitter wrote:
> >> Yo
> >> 
> >> On 18.10.20 10:11, Milian Wolff wrote:
> >>> Hey all,
> >>> 
> >>> since some time now I'm only compiling parts of KF5 selectively into a
> >>> custom prefix. Most notably, I'm only interested in ktexteditor and
> >>> syntax- highlighting, and would like to obtain all other frameworks
> >>> through my distribution packages.
> >>> 
> >>> Sadly, this doesn't work as easily, as ktexteditor is trying to do this:
> >>> 
> >>> ```
> >>> install(TARGETS kauth_ktexteditor_helper DESTINATION $
> >>> {KAUTH_HELPER_INSTALL_DIR} )
> >>> kauth_install_helper_files(kauth_ktexteditor_helper
> >>> org.kde.ktexteditor.katetextbuffer root)
> >>> kauth_install_actions(org.kde.ktexteditor.katetextbuffer buffer/
> >>> org.kde.ktexteditor.katetextbuffer.actions)
> >>> ```
> >>> 
> >>> Because my KAuth is a system-provided installation, and KTextEditor
> >>> should
> >>> be installed into a user-writable prefix, I get this error on `ninja
> >>> install`:
> >>> 
> >>> ```
> >>> 
> >>> CMake Error at src/cmake_install.cmake:143 (file):
> >>>   file INSTALL cannot copy file
> >>>   "/home/milian/projects/kf5/build/frameworks/ktexteditor/src/
> >>> 
> >>> org.kde.ktexteditor.katetextbuffer.policy"
> >>> 
> >>>   to
> >>>   "/usr/share/polkit-1/actions/org.kde.ktexteditor.katetextbuffer.policy
> >>>   "
> >>>   
> >>>   Permission denied.
> >>> 
> >>> Call Stack (most recent call first):
> >>>   cmake_install.cmake:77 (include)
> >>> 
> >>> ```
> >>> 
> >>> I can workaround this by either commenting out the kauth install bits in
> >>> ktexteditor or by installing kauth seperately too. But both are in my
> >>> opinion not ideal.
> >>> 
> >>> I don't think it's currently possible to overwrite the KAuth install
> >>> directory at cmake configure time. I would like to change that, unless
> >>> there is something I'm missing which would prevent this? I am hoping
> >>> that
> >>> any folder works as long as it's listed in the `XDG_DATA_DIRS` env var -
> >>> can someone confirm this?
> >> 
> >> Alas, the path is hardcoded at buildtime from what I see:
> >> https://gitlab.freedesktop.org/polkit/polkit/-/blob/master/src/polkitback
> >> end /polkitbackendinteractiveauthority.c#L302
> >> 
> >> Relevant: https://bugs.kde.org/show_bug.cgi?id=425272
> > 
> > So the path that needs to be used is actually the polkit path?
> 
> That's my understanding of the code, yes.
>
> > Meaning the
> > question where KAuth is being installed to doesn't really matter? That
> > would imply that KAuth is broken anyways when it's not installed to the
> > same prefix that polkit is being installed to, no?
> 
> Nope. If the policy was installed to a different path it would be
> broken, but the very error you posted is because kauth is insisting on
> putting the policy in the correct path rather than the prefix the rest
> of the build is installing to.

No, it's using the install location of kauth, it doesn't query polkit itself. 
I.e. if I hand-compile KAuth and install into a custom prefix, then I will 
have:

```
$ grep INSTALL /home/milian/projects/compiled/kf5-dbg/lib/cmake/KF5Auth/
KF5AuthConfig.cmake
set(KAUTH_POLICY_FILES_INSTALL_DIR "/home/milian/projects/compiled/kf5/share/
polkit-1/actions")
set(KAUTH_HELPER_INSTALL_DIR "lib/libexec/kauth")
set(KAUTH_HELPER_INSTALL_ABSOLUTE_DIR "/home/milian/projects/compiled/kf5/lib/
libexec/kauth")
```

Now I can happily compile e.g. ktexteditor against this, but it would be just 
as broken as the polkit files it installed will never be found anyways.

So, I guess there are two things to solve here:

a) make it easy to opt-out / disable kauth polkit file installation via a 
cache var in the `kauth_install_*` cmake macros, such that I can just disable 
that when building ktexteditor.

b) find a way to query polkit for the right path to use and use that by 
default instead of the kauth install prefix.

Or?
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: CMake kauth_install_* with system kauth and user CMAKE_INSTALL_PREFIX

[Milian Wolff]

On Montag, 19. Oktober 2020 11:27:29 CEST Harald Sitter wrote:
> Yo
> 
> On 18.10.20 10:11, Milian Wolff wrote:
> > Hey all,
> > 
> > since some time now I'm only compiling parts of KF5 selectively into a
> > custom prefix. Most notably, I'm only interested in ktexteditor and
> > syntax- highlighting, and would like to obtain all other frameworks
> > through my distribution packages.
> > 
> > Sadly, this doesn't work as easily, as ktexteditor is trying to do this:
> > 
> > ```
> > install(TARGETS kauth_ktexteditor_helper DESTINATION $
> > {KAUTH_HELPER_INSTALL_DIR} )
> > kauth_install_helper_files(kauth_ktexteditor_helper
> > org.kde.ktexteditor.katetextbuffer root)
> > kauth_install_actions(org.kde.ktexteditor.katetextbuffer buffer/
> > org.kde.ktexteditor.katetextbuffer.actions)
> > ```
> > 
> > Because my KAuth is a system-provided installation, and KTextEditor should
> > be installed into a user-writable prefix, I get this error on `ninja
> > install`:
> > 
> > ```
> > 
> > CMake Error at src/cmake_install.cmake:143 (file):
> >   file INSTALL cannot copy file
> >   "/home/milian/projects/kf5/build/frameworks/ktexteditor/src/
> > 
> > org.kde.ktexteditor.katetextbuffer.policy"
> > 
> >   to
> >   "/usr/share/polkit-1/actions/org.kde.ktexteditor.katetextbuffer.policy"
> >   :
> >   Permission denied.
> > 
> > Call Stack (most recent call first):
> >   cmake_install.cmake:77 (include)
> > 
> > ```
> > 
> > I can workaround this by either commenting out the kauth install bits in
> > ktexteditor or by installing kauth seperately too. But both are in my
> > opinion not ideal.
> > 
> > I don't think it's currently possible to overwrite the KAuth install
> > directory at cmake configure time. I would like to change that, unless
> > there is something I'm missing which would prevent this? I am hoping that
> > any folder works as long as it's listed in the `XDG_DATA_DIRS` env var -
> > can someone confirm this?
> 
> Alas, the path is hardcoded at buildtime from what I see:
> https://gitlab.freedesktop.org/polkit/polkit/-/blob/master/src/polkitbackend
> /polkitbackendinteractiveauthority.c#L302
> 
> Relevant: https://bugs.kde.org/show_bug.cgi?id=425272

So the path that needs to be used is actually the polkit path? Meaning the 
question where KAuth is being installed to doesn't really matter? That would 
imply that KAuth is broken anyways when it's not installed to the same prefix 
that polkit is being installed to, no?

If so, then it's just one more reason to be able to skip kauth policy 
installation, as they won't be picked up anyways...

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

CMake kauth_install_* with system kauth and user CMAKE_INSTALL_PREFIX

[Milian Wolff]

Hey all,

since some time now I'm only compiling parts of KF5 selectively into a custom 
prefix. Most notably, I'm only interested in ktexteditor and syntax-
highlighting, and would like to obtain all other frameworks through my 
distribution packages.

Sadly, this doesn't work as easily, as ktexteditor is trying to do this:

```
install(TARGETS kauth_ktexteditor_helper DESTINATION $
{KAUTH_HELPER_INSTALL_DIR} )
kauth_install_helper_files(kauth_ktexteditor_helper 
org.kde.ktexteditor.katetextbuffer root)
kauth_install_actions(org.kde.ktexteditor.katetextbuffer buffer/
org.kde.ktexteditor.katetextbuffer.actions)
```

Because my KAuth is a system-provided installation, and KTextEditor should be 
installed into a user-writable prefix, I get this error on `ninja install`:

```
CMake Error at src/cmake_install.cmake:143 (file):
  file INSTALL cannot copy file
  "/home/milian/projects/kf5/build/frameworks/ktexteditor/src/
org.kde.ktexteditor.katetextbuffer.policy"
  to "/usr/share/polkit-1/actions/org.kde.ktexteditor.katetextbuffer.policy":
  Permission denied.
Call Stack (most recent call first):
  cmake_install.cmake:77 (include)
```

I can workaround this by either commenting out the kauth install bits in 
ktexteditor or by installing kauth seperately too. But both are in my opinion 
not ideal.

I don't think it's currently possible to overwrite the KAuth install directory 
at cmake configure time. I would like to change that, unless there is 
something I'm missing which would prevent this? I am hoping that any folder 
works as long as it's listed in the `XDG_DATA_DIRS` env var - can someone 
confirm this?

Thanks

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: KPluginLoader UBSAN warnings (object has invalid vptr)

[Milian Wolff]

On Samstag, 17. Oktober 2020 00:16:01 CEST Thiago Macieira wrote:
> On Thursday, 15 October 2020 07:22:59 PDT Milian Wolff wrote:
> > I have the feeling that this might be a limitation of UBSAN? Or is this an
> > actual problem - does anyone know?
> 
> I've seen similar warnings that were impossible. See
> https://bugreports.qt.io/browse/QTBUG-85398
> 
> I'm not sure yet if this is an UBSan bug or if there was some weird,
> duplicate symbol problem.

Mysterious...

I'm using GCC 10.2.0 and only KDevelop was build with

CMAKE_BUILD_TYPE=Debug
CMAKE_CXX_FLAGS_DEBUG=-Og -g -fsanitize=address,undefined
CMAKE_C_FLAGS_DEBUG=-Og -g -fsanitize=address,undefined

All other dependencies, most notably Qt and the KDE frameworks are provided 
from Archlinux distro packages.

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

KPluginLoader UBSAN warnings (object has invalid vptr)

[Milian Wolff]

Hey all,

I'm finally taking a bit of time to look after KDevelop again. I would most 
notably like to make it ASAN/UBSAN clean. One thing I'm stumbling over are the 
following reports:

```
/usr/include/KF5/KCoreAddons/kpluginfactory.h:545:24: runtime error: member 
call on address 0x603f2d40 which does not point to an object of type 
'KPluginFactory'
0x603f2d40: note: object has invalid vptr
 33 00 80 0f  e0 31 d4 c3 5d 7f 00 00  a0 41 04 00 80 60 00 00  70 2d 0f 00 30 
60 00 00  00 00 00 00
  ^~~
  invalid vptr
#0 0x7f5dede47d8c in KDevelop::IPlugin* 
KPluginFactory::create(QObject*, QList const&) /
usr/include/KF5/KCoreAddons/kpluginfactory.h:545
#1 0x7f5dede47d8c in 
KDevelop::PluginController::loadPluginInternal(QString const&) /home/milian/
projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/shell/
plugincontroller.cpp:615
```

Or this one:

```
/usr/include/qt/QtCore/qobject.h:524:12: runtime error: downcast of address 
0x6060002922e0 which does not point to an object of type 'IPlugin'
0x6060002922e0: note: object has invalid vptr
 36 00 80 24  b0 2f d4 c3 5d 7f 00 00  a0 42 04 00 80 60 00 00  b0 30 d4 c3 5d 
7f 00 00  80 fe 06 00
  ^~~
  invalid vptr
#0 0x7f5dede47f20 in KDevelop::IPlugin* 
qobject_cast(QObject*) /usr/include/qt/QtCore/qobject.h:
524
#1 0x7f5dede47f20 in KDevelop::IPlugin* 
KPluginFactory::create(QObject*, QList const&) /
usr/include/KF5/KCoreAddons/kpluginfactory.h:547
```

I have the feeling that this might be a limitation of UBSAN? Or is this an 
actual problem - does anyone know?

Most notably, the kplugin* tests in kcoreaddons are UBSAN clean for me, which 
is quite odd. I would expect them to raise similar warnings, but apparently 
they don't. Or potentially it's simply that KDevelop plugins are way more 
complex - we apparently are using multiple inheritance there for example:

```
class IPlugin : public QObject, public KXMLGUIClient
class AStylePlugin : public KDevelop::IPlugin, public 
KDevelop::ISourceFormatter
```

Maybe that's the problem? Does anyone know?

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: Supported C++ standard (aka compiler version) Overview Table for various distros?

[Milian Wolff]

On Mittwoch, 14. Oktober 2020 12:07:04 CEST Neal Gompa wrote:
> On Wed, Oct 14, 2020 at 5:33 AM Milian Wolff  wrote:
> > Hey all,
> > 
> > I would like to push for C++17 usage in KDevelop to make it more fun to
> > develop there. This is an extragear app, so I believe we can take this
> > decision separately from the surrounding KDE ecosystem.
> > 
> > Does anyone know if there's an overview page similar to [1], but for Linux
> > distributions?
> > 
> > [1]: https://en.cppreference.com/w/cpp/compiler_support
> > 
> > I.e. I would like to know which distro versions we would potentially lose
> > by moving KDevelop master to C++17. I seem to recall that there existed
> > such an overview page somewhere, but I cannot find it anymore.
> > 
> > If it doesn't exist, then I would love if we could all together create
> > such a table on our techbase wiki.
> 
> Fedora 32 and 33 are using GCC 10, and Fedora 34 will be using GCC 11.
> 
> Red Hat Enterprise Linux/CentOS 8 is using GCC 8, but there is access
> to GCC 9 and soon GCC 10 too.
> 
> openSUSE Leap 15.x is using GCC 7. openSUSE Tumbleweed is using GCC 10
> right now.
> 
> Mageia 8 will be using GCC 8.
> 
> OpenMandriva Lx is currently using Clang 9 (and GCC 9).
> 
> I think you'll be generally fine.

Perfect, thank you!

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Supported C++ standard (aka compiler version) Overview Table for various distros?

[Milian Wolff]

Hey all,

I would like to push for C++17 usage in KDevelop to make it more fun to 
develop there. This is an extragear app, so I believe we can take this 
decision separately from the surrounding KDE ecosystem.

Does anyone know if there's an overview page similar to [1], but for Linux 
distributions?

[1]: https://en.cppreference.com/w/cpp/compiler_support

I.e. I would like to know which distro versions we would potentially lose by 
moving KDevelop master to C++17. I seem to recall that there existed such an 
overview page somewhere, but I cannot find it anymore.

If it doesn't exist, then I would love if we could all together create such a 
table on our techbase wiki.

Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: KDiagram - Persistent FTBFS for stable branch on Windows

[Milian Wolff]

On Dienstag, 13. Oktober 2020 09:03:01 CEST Dag wrote:
> mandag den 12. oktober 2020 21.02.31 CEST skrev Johnny Jazeix:
> > Hi,
> > 
> > issue is there:
> > https://invent.kde.org/graphics/kdiagram/-/blob/2.7/src/KChart/CMakeLists.
> > txt#L126 qt5_wrap_cpp(KChart kchart_LIB_SRCS KChartEnums.h) should be
> > qt5_wrap_cpp(kchart_LIB_SRCS KChartEnums.h)
> > 
> > I'm fixing it.
> 
> Thanks for spoting it, my eyesight is ok but my brain takes no notice...
> Mvh Dag

In general, consider enabling all of the CMAKE_AUTO{MOC,RCC,UI} variables and 
then stop using the manual qt5_wrap macros.

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: KDiagram - Persistent FTBFS for stable branch on Windows

[Milian Wolff]

On Montag, 12. Oktober 2020 14:04:06 CEST Dag wrote:
> mandag den 12. oktober 2020 12.58.36 CEST skrev Dag:
> > mandag den 12. oktober 2020 12.04.04 CEST skrev Milian Wolff:
> >> On Montag, 12. Oktober 2020 11:28:45 CEST Ben Cooksley wrote:
> >>> On Mon, Oct 12, 2020 at 10:24 PM Milian Wolff
> >>>  wrote: ...
> >> 
> >> Thanks, but that's looking worse then I expected. I guess
> >> backporting `35e86e964908ee906dde4f0678c16a838e4712dd` is worth
> >> a shot.
> >> 
> >> @Dag: what do you say, should that be safe enough to do?
> > 
> > No, it will not apply cleanly. I'll do it.
> 
> Done, but made no difference, still fails.
> Also tried qt_wrap_cpp -> qt5_wrap_cpp w/o sucess.
> 
> I'm clueless on windows and can't run any virtual on my machine.
> 
> Afaics we have this situation, please comment if wrong:
> - The windows build worked earlier, prior to move to invent.
> - There has been no changes to the stable branch between the successful
> build and the failed build.
> - At the first failing build of stable (oct 6), the code that seems to
> trigger the failure was the same in stable and unstable except for the
> qt5_wrap_cpp.
> - There has been some modernization of unstable, mostly Q_FOREACH() ->
> for()
> 
> I'm at loss of even where to start to look, so I hope somebody can point a
> finger in the right direction.

Try to add a src/KChart/KChartEnums.cpp file - I actually wonder how this 
compiles at all on non-windows platforms :)

Also add this line to the .h:

```
~KChartEnums();
```

and then this in the .cpp:

```
#include "KChartEnums.h"

KChartEnums::~KChartEnums() = default;
```

This should ensure that moc knows where to put the include for the generated 
code (into KChartEnums.cpp).

Cheers
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: KDiagram - Persistent FTBFS for stable branch on Windows

[Milian Wolff]

On Montag, 12. Oktober 2020 11:28:45 CEST Ben Cooksley wrote:
> On Mon, Oct 12, 2020 at 10:24 PM Milian Wolff  wrote:
> > On Montag, 12. Oktober 2020 11:11:22 CEST Ben Cooksley wrote:
> > > Hi KDiagram Developers,
> > > 
> > > The stable branch of KDiagram has been persistently failing to build
> > > from
> > > source now on Windows for a period greater than 5 days.
> > 
> > Hey Ben,
> 
> Hi Milian,
> 
> > can you link me to such a CI failure? Then I could try to blind-fix it, as
> > I
> > don't have a KDE-on-windows setup here. But maybe it's simple enough to
> > fix
> > even without that.
> 
> I suspect the fix just needs cherry picking - hopefully it isn't too
> complicated to fix.
> 
> The failure log is at
> https://build.kde.org/job/Calligra/job/kdiagram/job/stable-kf5-qt5%20Windows
> MSVCQt5.15/4/consoleText More recent builds can be found at
> https://build.kde.org/job/Calligra/job/kdiagram/job/stable-kf5-qt5%20Windows
> MSVCQt5.15/

Thanks, but that's looking worse then I expected. I guess backporting 
`35e86e964908ee906dde4f0678c16a838e4712dd` is worth a shot.

@Dag: what do you say, should that be safe enough to do?

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: KDiagram - Persistent FTBFS for stable branch on Windows

[Milian Wolff]

On Montag, 12. Oktober 2020 11:28:45 CEST Ben Cooksley wrote:
> On Mon, Oct 12, 2020 at 10:24 PM Milian Wolff  wrote:
> > On Montag, 12. Oktober 2020 11:11:22 CEST Ben Cooksley wrote:
> > > Hi KDiagram Developers,
> > > 
> > > The stable branch of KDiagram has been persistently failing to build
> > > from
> > > source now on Windows for a period greater than 5 days.
> > 
> > Hey Ben,
> 
> Hi Milian,
> 
> > can you link me to such a CI failure? Then I could try to blind-fix it, as
> > I
> > don't have a KDE-on-windows setup here. But maybe it's simple enough to
> > fix
> > even without that.
> 
> I suspect the fix just needs cherry picking - hopefully it isn't too
> complicated to fix.
> 
> The failure log is at
> https://build.kde.org/job/Calligra/job/kdiagram/job/stable-kf5-qt5%20Windows
> MSVCQt5.15/4/consoleText More recent builds can be found at
> https://build.kde.org/job/Calligra/job/kdiagram/job/stable-kf5-qt5%20Windows
> MSVCQt5.15/

Thanks, but that's looking worse then I expected. I guess backporting 
`35e86e964908ee906dde4f0678c16a838e4712dd` is worth a shot.

@Dag: what do you say, should that be safe enough to do?

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: KDiagram - Persistent FTBFS for stable branch on Windows

[Milian Wolff]

On Montag, 12. Oktober 2020 11:11:22 CEST Ben Cooksley wrote:
> Hi KDiagram Developers,
> 
> The stable branch of KDiagram has been persistently failing to build from
> source now on Windows for a period greater than 5 days.

Hey Ben,

can you link me to such a CI failure? Then I could try to blind-fix it, as I 
don't have a KDE-on-windows setup here. But maybe it's simple enough to fix 
even without that.

Cheers

> It would be appreciated if you could please remedy this situation without
> delay, as your failure prevents the CI system from operating services
> correctly for the following projects:
> - KStars
> - Konversation
> - Skrooge
> - Okteta
> - KDiff3
> - Alkimia
> - KMyMoney
> - KRename

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: KDiagram - Persistent FTBFS for stable branch on Windows

[Milian Wolff]

On Montag, 12. Oktober 2020 11:11:22 CEST Ben Cooksley wrote:
> Hi KDiagram Developers,
> 
> The stable branch of KDiagram has been persistently failing to build from
> source now on Windows for a period greater than 5 days.

Hey Ben,

can you link me to such a CI failure? Then I could try to blind-fix it, as I 
don't have a KDE-on-windows setup here. But maybe it's simple enough to fix 
even without that.

Cheers

> It would be appreciated if you could please remedy this situation without
> delay, as your failure prevents the CI system from operating services
> correctly for the following projects:
> - KStars
> - Konversation
> - Skrooge
> - Okteta
> - KDiff3
> - Alkimia
> - KMyMoney
> - KRename

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

D28039: optimize dynamic regex matching

[Milian Wolff]

mwolff added a comment.


  Let me try to explain the skip offset idea (it's been years since I came up 
with this in GeSHi :) )
  
  A code highlighter will repeatedly ask all highlight contexts and items 
therein to find the closest token to highlight next to the current cursor 
position.
  The closest token will win and then the highlighter will repeat its question 
at the position after the token.
  For regular expressions, it's often cheaper (as indicated by this patch once 
again), to match the line once starting from the current position and then 
remember where the first match - if any - is in the current line.
  Then, the next time the highlighter asks for a token position, we can check 
the last matched position. This is essentially the skip offset - i.e. we know 
that we can skip querying the regexp again until the cursor position is beyond 
the next match.
  
  Dominik, does this clear things up?

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  master

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

To: cullmann, dhaumann, vkrause, nibags
Cc: mwolff, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, 
GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D27065: Fix inverted logic in IOKitStorage::isRemovable

[Milian Wolff]

This revision was automatically updated to reflect the committed changes.
Closed by commit R245:d2172d3c0843: Fix inverted logic in 
IOKitStorage::isRemovable (authored by mwolff).

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27065?vs=74772=75857

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

AFFECTED FILES
  src/solid/devices/backends/iokit/iokitstorage.cpp

To: mwolff, #frameworks, rjvbb, cgilles
Cc: kde-mac, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27285: Add left/right indent fill (as opposed to left-only), extend indent lines to broken lines

[Milian Wolff]

mwolff added a comment.


  In D27285#611163 , @dhaumann wrote:
  
  > Ok, I now fully got this, thanks for the explanation. And indeed without 
any left fill visual feedback is missing.
  >
  > What I wonder is how much of this needs to be configurable. We could also 
just make the left fill of 4 default or so.
  >
  > In any case, I think we should go forward with this patch. @cullmann your 
opinion?
  
  
  You could use the current document's indent width instead of hardcoding 4?

REPOSITORY
  R39 KTextEditor

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

To: eudoxos, #vdg
Cc: mwolff, cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, 
LeGast00n, cblack, GB_2, domson, michaelh, ngraham, bruns, demsking, sars

D27065: Fix inverted logic in IOKitStorage::isRemovable

[Milian Wolff]

mwolff added a comment.


  ping? any update?

REPOSITORY
  R245 Solid

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

To: mwolff, #frameworks, rjvbb, cgilles
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[Milian Wolff]

mwolff added a comment.


  In D27065#608920 , @rjvbb wrote:
  
  > How do you connect? The Mac OS has a built-in VNC server but it has to be 
activated. Once it is you should be able to connect using any VNC client 
(possibly using ssh tunnelling?).
  
  
  Yes, I'm connected via VNC and ssh.
  
  > You can configure the Finder to show every connected volume on the desktop, 
IIRC it used to be the default that it shows everything. Should be in the 
Preferences that you can find under the application menu (the one immediately 
to the right of the Apple icon/menu).
  
  F8095364: mac_disks.png 
  
  > Can you please ask the owner how that drive is connected? The SM0256F is a 
PCIe drive; according to 
https://www.anandtech.com/show/9136/the-2015-macbook-review/8 `In the 2013 
MacBook Air 13” for example the drive model was “SM0256F”`.
  
  This is a MacBook Pro Retina, 13-inch, Mid 2014 with macOS 10.15.2 with only 
this one disk. It's clearly not supposed to get removed, imo :)

REPOSITORY
  R245 Solid

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

To: mwolff, #frameworks, rjvbb, cgilles
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[Milian Wolff]

mwolff added a comment.


  I now got remote access to a macOS machine. That's where I now got the output 
from. How do I see all drives in finder? I.e. how could I make the association 
between the `solid-hardware5` output and finder?

REPOSITORY
  R245 Solid

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

To: mwolff, #frameworks, rjvbb, cgilles
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[Milian Wolff]

mwolff added a comment.


  In D27065#608818 , @rjvbb wrote:
  
  > In D27065#608746 , @mwolff wrote:
  >
  > > before:
  >
  >
  > [snip]
  >
  > > Note the `Ejectable = false  (bool)` vs. `StorageDrive.removable = true  
(bool)`. The patch here fixes it to yield `StorageDrive.removable = false  
(bool)`
  >
  > Where does this output come from, and what kind of drive does it concern 
(apart from some kind of Apple-branded SSD)? In other words, should `removable` 
be false, or should `ejectable` be true? I concur that the status quo appears 
inappropriate, at least in "Finder speak" which uses "Eject" for any form of 
unmounting regardless of whether it implies a physical eject.
  
  
  The output comes from running `solid-hardware5 details` and `solid-hardware 
nonportableinfo` on the UID.
  
  > In principle my IOKit code worked appropriately with the various drives I 
have at my disposal (USB, Firewire, even an internal optical drive) and as far 
as Solid and IOKit API/protocols can be mapped to one another. Digikam 
identifies removable drives correctly, for instance.
  > 
  > The logic in summary here seems correct but it's not impossible that I 
implemented one or two counterintuitive things in order to make things behave 
the way Mac users would expect. I'll try to take a look this week but maybe 
Gilles can actually test with a Thunderbolt enclosure?

REPOSITORY
  R245 Solid

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

To: mwolff, #frameworks, rjvbb, cgilles
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[Milian Wolff]

mwolff added a comment.


  before:
  
udi = 
'IOService:/AppleACPIPlatformExpert/PCI0@0/AppleACPIPCI/RP06@1C,5/IOPP/SSD0@0/AppleAHCI/PRT0@0/IOAHCIDevice@0/AppleAHCIDiskDriver/IOAHCIBlockStorageDevice/IOBlockStorageDriver/APPLE
 SSD SM0256F Media'
  parent = 
'IOService:/AppleACPIPlatformExpert/PCI0@0/AppleACPIPCI/RP06@1C,5/IOPP/SSD0@0/AppleAHCI/PRT0@0/IOAHCIDevice@0/AppleAHCIDiskDriver/IOAHCIBlockStorageDevice/IOBlockStorageDriver'
  (string)
  vendor = ''  (string)
  product = 'APPLE SSD SM0256F   '  (string)
  description = 'APPLE SSD SM0256F Media'  (string)
  icon = 'drive-removable-media'  (string)
  Block.major = 1  (0x1)  (int)
  Block.minor = 0  (0x0)  (int)
  Block.device = '/dev/disk0'  (string)
  StorageAccess.accessible = false  (bool)
  StorageAccess.filePath = ''  (string)
  StorageAccess.ignored = false  (bool)
  StorageDrive.bus = 'Platform'  (0x5)  (enum)
  StorageDrive.driveType = 'HardDisk'  (0x0)  (enum)
  StorageDrive.removable = true  (bool)
  StorageDrive.hotpluggable = false  (bool)
  StorageDrive.inUse = true  (bool)
  StorageDrive.size = 251000193024  (0x3a70c7)  (qulonglong)
  StorageVolume.ignored = false  (bool)
  StorageVolume.usage = 'PartitionTable'  (0x3)  (enum)
  StorageVolume.fsType = ''  (string)
  StorageVolume.label = ''  (string)
  StorageVolume.uuid = ''  (string)
  StorageVolume.size = 251000193024  (0x3a70c7)  (qulonglong)

udi = 
'IOService:/AppleACPIPlatformExpert/PCI0@0/AppleACPIPCI/RP06@1C,5/IOPP/SSD0@0/AppleAHCI/PRT0@0/IOAHCIDevice@0/AppleAHCIDiskDriver/IOAHCIBlockStorageDevice/IOBlockStorageDriver/APPLE
 SSD SM0256F Media'
  BSD Major = 1  (0x1)  (int)
  BSD Minor = 0  (0x0)  (int)
  BSD Name = 'disk0'  (string)
  BSD Unit = 0  (0x0)  (int)
  Content = 'GUID_partition_scheme'  (string)
  Content Hint = ''  (string)
  Ejectable = false  (bool)
  IOBusyInterest = 'IOCommand is not serializable'  (string)
  IOGeneralInterest = 'IOCommand is not serializable'  (string)
  IOMediaIcon = ''  (string)
  IOPolledInterfaceStack = 'IOPolledFilePollers is not serializable'  
(string)
  Leaf = false  (bool)
  Open = true  (bool)
  Preferred Block Size = 512  (0x200)  (qlonglong)
  Removable = false  (bool)
  Size = 251000193024  (0x3a70c7)  (qlonglong)
  Whole = true  (bool)
  Writable = true  (bool)
  className = 'IOMedia'  (string)
  
  Note the `Ejectable = false  (bool)` vs. `StorageDrive.removable = true  
(bool)`. The patch here fixes it to yield `StorageDrive.removable = false  
(bool)`

REPOSITORY
  R245 Solid

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

To: mwolff, #frameworks
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[Milian Wolff]

mwolff added a reviewer: rjvbb.

REPOSITORY
  R245 Solid

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

To: mwolff, #frameworks, rjvbb
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D22477: With auto completion don't show completions that don't match from beginning of typed word

[Milian Wolff]

mwolff added a comment.


  I agree, this is a feature, not a bug. If people are annoyed, they are 
usually annoyed by other issues like the one shown by Sven

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, #kdevelop, kossebau, mwolff, 
kfunk
Cc: brauch, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, GB_2, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D27065: Fix inverted logic in IOKitStorage::isRemovable

[Milian Wolff]

mwolff added a reviewer: Frameworks.

REPOSITORY
  R245 Solid

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

To: mwolff, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[Milian Wolff]

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

REVISION SUMMARY
  Reading through the code, I realized that the isRemovable check
  returned true when the kDADiskDescriptionDeviceInternalKey property
  is set to true. But that sounds like the check needs to be inverted:
  
  According to [1] e.g. a disk is non-removable when it is internal.
  And kDADiskDescriptionDeviceInternalKey returns whether the disk
  is internal, not external.
  
  [1]: 
https://stackoverflow.com/questions/38499860/thunderbolt-drives-not-marked-as-ejectable-in-disk-arbitration-iokit-although-th#comment64407405_38499860
  
  I do not own a device with IOKit platfor, so I cannot actually test
  whether this patch is correct or not.

REPOSITORY
  R245 Solid

BRANCH
  master

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

AFFECTED FILES
  src/solid/devices/backends/iokit/iokitstorage.cpp

To: mwolff
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

Re: KInit - Current state and benchmarks

[Milian Wolff]

On Montag, 25. November 2019 22:57:11 CET Albert Astals Cid wrote:
> El dissabte, 23 de novembre de 2019, a les 11:47:40 CET, Milian Wolff va 
escriure:
> > On Mittwoch, 19. Juni 2019 19:57:56 CET Albert Astals Cid wrote:
> > > El dimarts, 18 de juny de 2019, a les 12:04:38 CEST, David Edmundson va
> > 
> > escriure:
> > > > > Are we sure it's fair to assume people have SSD? our of the 4
> > > > > laptops i
> > > > > own, only 2 have SSD.>
> > > > 
> > > > It's at least safe to assume it's the trend moving forward.
> > > > 
> > > > > Do you think it's worth me trying in one of the two that don't have
> > > > > SSD?
> > > > 
> > > > More data is normally a good thing. If you or anyone else wants to
> > > > collect stats:
> > > > From my git link above, it's as simple as running the normal ; cmake;
> > > > make ; ./kinittest -median 5
> > > 
> > > On my very old/very slow computer seems to make a lot of difference
> > > 
> > > RESULT : DaveTest::testQProcess():
> > >  2,625 msecs per iteration (total: 2,625, iterations: 1)
> > > 
> > > RESULT : DaveTest::testKInit():
> > >  1,852 msecs per iteration (total: 1,852, iterations: 1)
> > > 
> > > RESULT : DaveTest::testQProcess():
> > >  2,390 msecs per iteration (total: 2,390, iterations: 1)
> > > 
> > > RESULT : DaveTest::testKInit():
> > >  1,846 msecs per iteration (total: 1,846, iterations: 1)
> > 
> > Hey Albert,
> > 
> > these numbers are quite impressive but I can't quite explain those. Are
> > you
> > measuring maybe a full debug build without any compiler optimizations?
> 
> I obviously can't remember, this was *months* ago, but i just ran the test
> again (making sure -O3 was there and not any -g) and got a bit different
> results, so maybe it was.

Does this apply to your whole stack (instead of just the inittest from Dave)?

> New results:
>  * testQProcess: 2200 msec per iteration
>  * testKInit:1700 msec per iteration
> 
> Still a 20% speed improvement.
> 
> > Then
> > the library sizes will be _much_ larger and thus trigger more page faults.
> > If every one of those is extremely slow on that machine compared to more
> > modern machines?
> > 
> > May I ask how old this machine is and what the speed of the HDD is?
> 
> It's a Lenovo ideapad S10-3t, around 10 years old, the HDD is slow. But it's
> of similar power to the Librecomputer La Frite i just got for free at
> LinuxAppSummit, so even this is on the slow end of things we support i
> think there's value on supporting it.
> 
> If you're interested i can arrange you to get ssh access to the machine (the
> ideapad, i don't have all the KF5/Qt stack built for the LaFrite).

Yes, that would be interesting for me!

Cheers
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

Re: KInit - Current state and benchmarks

[Milian Wolff]

On Mittwoch, 19. Juni 2019 19:57:56 CET Albert Astals Cid wrote:
> El dimarts, 18 de juny de 2019, a les 12:04:38 CEST, David Edmundson va 
escriure:
> > > Are we sure it's fair to assume people have SSD? our of the 4 laptops i
> > > own, only 2 have SSD.> 
> > It's at least safe to assume it's the trend moving forward.
> > 
> > > Do you think it's worth me trying in one of the two that don't have SSD?
> > 
> > More data is normally a good thing. If you or anyone else wants to
> > collect stats:
> > From my git link above, it's as simple as running the normal ; cmake;
> > make ; ./kinittest -median 5
> 
> On my very old/very slow computer seems to make a lot of difference
> 
> RESULT : DaveTest::testQProcess():
>  2,625 msecs per iteration (total: 2,625, iterations: 1)
> RESULT : DaveTest::testKInit():
>  1,852 msecs per iteration (total: 1,852, iterations: 1)
> 
> 
> RESULT : DaveTest::testQProcess():
>  2,390 msecs per iteration (total: 2,390, iterations: 1)
> RESULT : DaveTest::testKInit():
>  1,846 msecs per iteration (total: 1,846, iterations: 1)

Hey Albert,

these numbers are quite impressive but I can't quite explain those. Are you 
measuring maybe a full debug build without any compiler optimizations? Then 
the library sizes will be _much_ larger and thus trigger more page faults. If 
every one of those is extremely slow on that machine compared to more modern 
machines?

May I ask how old this machine is and what the speed of the HDD is?

Thanks

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

D24568: Provide clang-format target with a KDE Frameworks style file

[Milian Wolff]

mwolff added a comment.


  In D24568#545942 , @cullmann wrote:
  
  > In D24568#545736 , @apol wrote:
  >
  > > I'm not sure how this works, but would it be possible to have a target 
that only works on a patch? You usually want to make sure what you modified 
didn't diverge from the code.
  >
  >
  > I think there is some hack around that:
  >  http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting
  >
  > But actually, if your sources are already clang-formatted, you just need to 
run the clang-format target once before you commit, the your new code will be 
the only thing altered.
  
  
  For our projects at work where we have the liberty to dictate the coding 
style, we also use clang format. To do that properly, we have a pre-commit 
check locally and then a gerrit bot similar to the Qt coding style bot which 
checks the formatting introduced by a patch. This way, one can be sure that the 
style doesn't deteriorate over time.
  
  Some links on that matter:
  
  - https://github.com/nghttp2/nghttp2/blob/master/pre-commit is what I use to 
check my commits, there are probably other, better hooks available somewhere, 
but this one has suited me well so far
  - when the hook complains, I run `git clang-format` which is part of the 
clang package that also ships clang-format on ArchLinux
  
  I guess the commit hook can also be used for a server check, but I'm not sure 
how this is done with gerrit for us, or how one would do it with 
phabricator/gitlab for KDE.
  
  Anyhow: big +1 form my side for using clang-format for all of KDE 
(eventually) and KDE frameworks in the near future as a starting point.

REPOSITORY
  R240 Extra CMake Modules

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

To: cullmann, #frameworks
Cc: mwolff, ochurlaud, nalvarez, kossebau, aacid, davidedmundson, dhaumann, 
apol, ognarb, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D17241: WIP:Disable highlighting after 512 characters on a line.

[Milian Wolff]

mwolff added a comment.


  I personally dislike this too.

INLINE COMMENTS

> katerenderer.cpp:408
> +auto  = renderRanges.pushNewRange();
>  for (int i = 0; i < al.count(); ++i)
>  if (al[i].length > 0 && al[i].attributeValue > 0) {

couldn't the al.count be limited here? if I understand the patch correctly, we 
throw out all highlighting if we encounter too many ranges. but could we 
instead just skip the items that are beyond the limit?

REPOSITORY
  R39 KTextEditor

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

To: cullmann, vkrause, dhaumann, mwolff, sars
Cc: zetazeta, mwolff, brauch, kwrite-devel, kde-frameworks-devel, LeGast00n, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

kdesrc-build messes with environment

[Milian Wolff]

Hey all,

for some reason, my `kdesrc-build` uses a different environment than my normal 
shells. I have so far not figured out why that is:

$ env | grep "^PATH="
PATH=/home/milian/.bin:/home/milian/projects/compiled/other/bin:/home/
milian/.bin/kf5:/home/milian/projects/compiled/kf5-dbg/bin:/home/milian/
projects/compiled/other/bin:/home/milian/projects/compiled/kf5/bin:/usr/local/
sbin:/usr/local/bin:/usr/bin:/usr/lib/jvm/default/bin:/usr/bin/site_perl:/usr/
bin/vendor_perl:/usr/bin/core_perl

$ kdesrc-build --run env | grep "^PATH="
PATH=/bin:/home/milian/.bin:/home/milian/projects/compiled/other/bin:/home/
milian/.bin/kf5:/home/milian/projects/compiled/kf5-dbg/bin:/home/milian/
projects/compiled/other/bin:/home/milian/projects/compiled/kf5/bin:/usr/local/
sbin:/usr/local/bin:/usr/bin:/usr/lib/jvm/default/bin:/usr/bin/site_perl:/usr/
bin/vendor_perl:/usr/bin/core_perl

Note how it prepends /bin to PATH, which leads to all kinds of nasty side 
effects for me. I want my PATH to be used as-is, most notably such that some 
of the tools I've built myself get picked up, rather than the versions I have 
available globally in /bin.

Does anyone know where this could come from?

Thanks
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

D20606: Toggle folding of child ranges by right click

[Milian Wolff]

mwolff added a comment.


  In D20606#451459 , @dhaumann wrote:
  
  > I would prefer a context menu that has this as action. This is much better 
discoverable and also extensible with more folding actions.
  
  
  I agree. Right-click should show a context menu. Middle-click could toggle, 
if you need it.

REPOSITORY
  R39 KTextEditor

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

To: loh.tar, #ktexteditor, cullmann, ngraham, dhaumann
Cc: mwolff, kwrite-devel, kde-frameworks-devel, #ktexteditor, domson, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

Re: Welcome CuteHMI

[Milian Wolff]

On Montag, 15. April 2019 13:42:47 CEST Michal Policht wrote:
> Hi all,
>  
> 
> > Do you plan to "fix" that now that qbs is "dead"?
> 
> Qbs is indeed an issue we have to "fix" somehow.
> 
> CMake files generator for Qbs would be handy, but unfortunately it's not
> there.

Sounds like the perfect idea for the next incubation project!

> TBH Qbs has some pros, like it performs builds really fast, it has
> powerful, declarative syntax, it does not require underlying build
> system, it can be easily extended with features through JavaScript... I
> wish the project wasn't dead, but I try to hang on and not cry :P

"all build systems suck", don't bother too much about it ;-)
-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

D19876: Fix: apply correctly the text colors of the chosen scheme

[Milian Wolff]

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


  one minor nit, otherwise looks like a good improvement

INLINE COMMENTS

> katehighlight.cpp:78
> +if (schema == QLatin1String("Normal")) {
> +return QLatin1String("Default");
> +} else if (schema == QLatin1String("Solarized (light)")) {

return QStringLiteral, otherwise you allocate on every function call (also 
below)

REPOSITORY
  R39 KTextEditor

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

To: nibags, #ktexteditor, #kate, cullmann, mwolff
Cc: mwolff, cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, gennad, 
domson, michaelh, ngraham, bruns, demsking, sars

Re: Gitlab Evaluation & Migration

[Milian Wolff]

On Donnerstag, 28. Februar 2019 07:02:03 CET Ben Cooksley wrote:
> On Thu, Feb 28, 2019 at 5:12 PM Michael Pyne  wrote:
> > On Wed, Feb 27, 2019 at 03:15:58PM -0700, Nate Graham wrote:
> > >   On Wed, 27 Feb 2019 12:12:55 -0700 Eike Hein  wrote
> > >  
> > >  
> > >  > On 2/27/19 4:38 AM, Nate Graham wrote:
> > >  > > It's really pretty nice. But  Gitlab has a
> > >  > > fork-the-repo-and-submit-a-merge-request workflow, so in steps 3
> > >  > > and 4, people without commit access won't just be able to start
> > >  > > hacking with the source checkout that kdesrc-build downloads, or
> > >  > > else they won't be able to push their branch to any remotes and
> > >  > > create a Merge Request.> >  > 
> > >  > No, this is not correct.
> > >  > 
> > >  > When you have a local git repository (be it your own or a clone), it
> > >  > can
> > >  > interact with any number of remote git repositories.
> > >  > 
> > >  > When you do `git clone `, git automatically adds
> > >  > 
> > >  > as a remote named "origin" to the local repository. But what "origin"
> > >  > points to can be changed at any time, or other remotes with other
> > >  > names
> > >  > can be added for pushing too. "origin" is just a convention.
> > >  > 
> > >  > I.e. someone can totally do this:
> > >  > 1. use kdesrc-build to clone stuff
> > >  > 2. git checkout -b feature to make a feature branch
> > >  > 3. hack
> > >  > 4. make a private fork on gitlab
> > >  > 5. push to their fork from the clone they've been working in
> > >  > 
> > >  > It's not necessary to fork first and clone your fork to get started.
> > > 
> > > Thanks Eike, that makes e a lot of sense. Going to the website to fork
> > > each repo for the first time still adds an additional manual step
> > > compared to the status quo, so it would be nice if we can get
> > > kdesrc-build so set up forks automatically. That would be really
> > > slick.
> > 
> > That would be slick. I wonder if Gitlab exposes an API for that (ideally
> > something that doesn't involve kdesrc-build having to store your creds)?
> > Potentially this API
> > https://docs.gitlab.com/ee/api/projects.html#fork-project (though it's
> > documented for EE not CE)?
> 
> The API for both EE and CE is identical, except for the functionality
> that is dependent on the edition of EE it is available in (which is
> only enabled if you are using that edition)
> With regards to credentials, you would need to give it an API Token yes.
> 
> In terms of server load, it would be nice if the setup of forks was
> still something the developer had to initiate rather than being done
> automatically for every repository touched by kdesrc-build (I say this
> mainly as if we had 50 people fork just half of the mainline
> repositories we have, that's ~450GB of space used up - a massive
> scalability issue)

Are you sure about this? Isn't gitlab using something like `git-new-workdir` 
internally to save on the disk overhead? If not, then request it, that would 
be an obvious optimization opportunity.

Bye

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

D18793: Handle text completion with block selection mode

[Milian Wolff]

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


  we override execution in our own completion models, so this patch will only 
change the behavior for the builtin word and keyword completion models in 
ktexteditor I believe
  
  that said, I think it makes sense to insert the word everywhere in block 
selection, it shouldn't be different from typing text.
  
  so +1 for the idea, but -1 on the actual implementation:
  
  - we need to have a unit test for this new behavior
  - we should introduce new helper API to make it easier to opt-in to this new 
behavior and reduce the if/else depth. This would also make it easier for us in 
KDevelop to change our behavior accordingly. I believe the code completion 
execution code should basically be agnostic to the block selection mode. I.e. 
instead of the proposed
  
if (completeBlockSelection) {
removeText
typeChars
} else {
replaceText
}
  
  it should always just call "replaceText" with the the block selection range 
and then internal API should duplicate the text, if the selection is a block 
selection
  
  as-is, this patch adds too much code duplication

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, #ktexteditor, cullmann, dhaumann, #kdevelop, mwolff
Cc: mwolff, kde-frameworks-devel, kwrite-devel, #ktexteditor, gennad, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D18612: Cache the default KColorScheme configuration

[Milian Wolff]

This revision was automatically updated to reflect the committed changes.
Closed by commit R265:c0cc6b8a200a: Cache the default KColorScheme 
configuration (authored by mwolff).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D18612?vs=50541=51212#toc

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18612?vs=50541=51212

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kcolorschemetest.cpp
  src/kcolorscheme.cpp

To: mwolff, #kate, #kdevelop, dfaure, broulik
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D18612: Cache the default KColorScheme configuration

[Milian Wolff]

mwolff added a comment.


  pushed this now with a proper benchmark too, shows a ~10x performance win 
when a non-empty PATH is set

REPOSITORY
  R265 KConfigWidgets

BRANCH
  master

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

To: mwolff, #kate, #kdevelop, dfaure, broulik
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D17932: Improvements to completion

[Milian Wolff]

mwolff added a comment.


  I'm in favor!

REPOSITORY
  R39 KTextEditor

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

To: thomassc, #ktexteditor, #kdevelop, mwolff
Cc: dhaumann, apol, kfunk, brauch, mwolff, cullmann, kwrite-devel, 
kde-frameworks-devel, hase, michaelh, ngraham, bruns, demsking, head7, sars

D18163: Set the color scheme to Printing for Print Preview

[Milian Wolff]

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


  yes, thanks!

REPOSITORY
  R39 KTextEditor

BRANCH
  print-preview-text-color (branched from master)

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

To: ahmadsamir, cullmann, #ktexteditor, dhaumann, mwolff
Cc: kwrite-devel, kde-frameworks-devel, hase, michaelh, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D18612: Cache the default KColorScheme configuration

[Milian Wolff]

mwolff added a comment.


  sure, but first let's get this in. @broulik or @dfaure care to give your +1?

REPOSITORY
  R265 KConfigWidgets

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

To: mwolff, #kate, #kdevelop, dfaure
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D18163: Set the color scheme to Printing when print preview'ing

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kateprinter.cpp:164
>  QPrinter printer;
> -KatePrinterPrivate p(view->doc(), view);
> +KatePrinterPrivate p(view->doc(), view, true);
>  QPrintPreviewDialog preview();

better add a setter before and then call

`p.setColorScheme(QStringLiteral("Printing"));`

here

> kateprinter.cpp:180
>  QPrinter printer;
> -KatePrinterPrivate p(doc);
> +KatePrinterPrivate p(doc, nullptr, true);
>  QPrintPreviewDialog preview();

dito

REPOSITORY
  R39 KTextEditor

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

To: ahmadsamir, cullmann, #ktexteditor, dhaumann, mwolff
Cc: kwrite-devel, kde-frameworks-devel, hase, michaelh, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D18612: Cache the default KColorScheme configuration

[Milian Wolff]

mwolff added a comment.


  I see that it's faster when I profile kate/kdev, but I cannot easily write a 
benchmark for this. It's only noticeable when the KDE_COLOR_SCHEME_PATH 
variable is set, otherwise the global application config will be used afte 
rall, which is going to be shared most probably. Any idea how I could construct 
a valid KDE_COLOR_SCHEME_PATH path to e.g. the breeze color scheme from within 
a kcolor scheme auto test?

REPOSITORY
  R265 KConfigWidgets

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

To: mwolff, #kate, #kdevelop, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D18612: Cache the default KColorScheme configuration

[Milian Wolff]

mwolff created this revision.
mwolff added reviewers: Kate, KDevelop, dfaure.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
mwolff requested review of this revision.

REVISION SUMMARY
  KDevelop, Kate and probably other applications too, recreate
  KColorScheme instances repeatedly. This was very costly since we
  ended up reparsing the internal color scheme configuration file
  every time - the shared configuration wasn't stored anywhere thus
  it's refcount dropped to zero after once the KColorScheme was
  fully constructed.
  
  Optimize this apparently common scenario by caching the configuration
  in a thread_local variable and only open a new configuration when the
  user changed the application color scheme.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  master

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

AFFECTED FILES
  src/kcolorscheme.cpp

To: mwolff, #kate, #kdevelop, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17137: KTextEditor: File menu: Put Save, Print and Export in submenus

[Milian Wolff]

mwolff resigned from this revision.

REPOSITORY
  R39 KTextEditor

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

To: gregormi, #kate, #kdevelop
Cc: loh.tar, anthonyfieroni, ngraham, cullmann, flherne, dhaumann, 
kwrite-devel, kde-frameworks-devel, hase, michaelh, bruns, demsking, sars

D17949: ViewPrivate: Make 'Apply Word Wrap' more comfortable

[Milian Wolff]

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


  please look at the existing tests and expand that. There's e.g. 
`KateDocumentTest::testWordWrap` in 
`ktexteditor/autotests/src/katedocument_test.cpp`
  
  also don't change the behavior of wordwrapping that is used by the view 
compared to what we would get by calling wordwrap on the document directly

INLINE COMMENTS

> kateview.cpp:2346
>  {
> -if (selection()) {
> -doc()->wrapText(selectionRange().start().line(), 
> selectionRange().end().line());
> -} else {
> -doc()->wrapText(0, doc()->lastLine());
> +doc()->editStart();
> +int first = selectionRange().start().line();

all of this functionality needs to go into the document, it shouldn't depend on 
the view.

REPOSITORY
  R39 KTextEditor

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

To: loh.tar, #ktexteditor, mwolff
Cc: dhaumann, cullmann, mwolff, kwrite-devel, kde-frameworks-devel, 
#ktexteditor, hase, michaelh, ngraham, bruns, demsking, sars

D17693: DocumentPrivate: Treat some chars also as "auto bracket" only when we have a selection

[Milian Wolff]

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


  Ok, that behavior you describe is bad. I would get annoyed by that, esp. 
since the list of chars that you use are totally language specific.
  
  If you want to get this behavior for Markdown, then we need to store that 
data in the syntax file and make it accessible at this position. in the 
`` section we already define how single/multiline comments are 
handled. We would need something similar for this feature, e.g. add something 
like the following and make it accessible through the syntax-highlighting API:
  

  































  

  
  Of course the initial, common, brackets should be the implicit default when 
no `` group is specified in a language file. And then we probably 
need more meta data that specified that some of the brackets above only should 
be added when we have a selection. Just add a `requiresSelection` or similar 
attribute?

REPOSITORY
  R39 KTextEditor

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

To: loh.tar, #ktexteditor, cullmann, mwolff
Cc: mwolff, cullmann, sars, kwrite-devel, kde-frameworks-devel, #ktexteditor, 
hase, michaelh, ngraham, bruns, demsking, dhaumann

D17693: DocumentPrivate: Treat some chars also as "auto bracket" only when we have a selection

[Milian Wolff]

mwolff added a comment.


  can you say what this exactly does? from reading the code and the somewhat 
vague commit message, it makes me believe that when I have anything selected 
and then press e.g. `?` the selection becomes wrapped in two `?` - is that 
right? when would we ever want this?
  
  and if we only want this for, say, markdown, then it should be a 
per-highlightfile list of chars that trigger this special behavior. In C++ e.g. 
I would hate if I couldn't just select code and replace it by an operator 
anymore, e.g. if I have `foo + bar` and I select the `+ bar` part and then 
start typing `- asdf`, would I now suddenly get `foo -+ bar- asdf`?!

REPOSITORY
  R39 KTextEditor

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

To: loh.tar, #ktexteditor, cullmann
Cc: mwolff, cullmann, sars, kwrite-devel, kde-frameworks-devel, #ktexteditor, 
hase, michaelh, ngraham, bruns, demsking, dhaumann

D17932: Improvements to completion

[Milian Wolff]

mwolff accepted this revision as: mwolff.
mwolff added a comment.


  @brauch, @kfunk what do you say?

REPOSITORY
  R39 KTextEditor

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

To: thomassc, #ktexteditor, #kdevelop, mwolff
Cc: apol, kfunk, brauch, mwolff, cullmann, kwrite-devel, kde-frameworks-devel, 
hase, michaelh, ngraham, bruns, demsking, head7, sars, dhaumann

D18163: KateRenderer: when printing initially set the color scheme to Printing

[Milian Wolff]

mwolff accepted this revision as: mwolff.
mwolff added a comment.
This revision is now accepted and ready to land.


  lgtm, @cullmann @dhaumann? could the schema name be translated (I hope not)?

REPOSITORY
  R39 KTextEditor

BRANCH
  print-preview-text-color

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

To: ahmadsamir, cullmann, #ktexteditor, dhaumann, mwolff
Cc: mwolff, kwrite-devel, kde-frameworks-devel, hase, michaelh, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D17949: ViewPrivate: Make applyWordWrap() more comfortable

[Milian Wolff]

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


  I like what I'm seeing in the screenshot, but please add proper tests for 
this functionality

INLINE COMMENTS

> kateview.cpp:2355
> +// Because we shrink and expand lines, we need a powerful "Moving Cursor"
> +KTextEditor::MovingCursor *curr = 
> doc()->newMovingCursor(KTextEditor::Cursor(selectionRange().start()));
> +

store in a std::unique_ptr and remove the manual `delete` further down below

> kateview.cpp:2359
> +for (int line = first; line <= selectionRange().end().line(); ++line) {
> +// Is our first line a somehow filled line?
> +while(doc()->plainKateTextLine(first)->firstChar() < 0) {

bool shouldWrap = true;

> kateview.cpp:2366
> +// C++ can only continue the inner loop, but we need here to 
> continue our "for" loop
> +goto NextLine;
> +}

shouldWrap = false;
break;

> kateview.cpp:2370
> +// Is our current line a somehow filled line? If not, wrap the 
> paragraph
> +if (doc()->plainKateTextLine(line)->firstChar() < 0) {
> +curr->setPosition(line, 0); // Set on empty line

if (!shouldWrap) {
  continue;
  }

REPOSITORY
  R39 KTextEditor

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

To: loh.tar, #ktexteditor, mwolff
Cc: mwolff, kwrite-devel, kde-frameworks-devel, #ktexteditor, hase, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D17128: WIP DocumentPrivate: Remove all from next line which may annoying when joining lines

[Milian Wolff]

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


  can you please rephrase the title of this review to make it understandable? 
"Remove all" - what do you remove? All what?
  
  Also, can you please add tests for the feature you are adding, this also acts 
as documentation on what this patch actually does

REPOSITORY
  R39 KTextEditor

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

To: loh.tar, #ktexteditor, cullmann, mwolff
Cc: mwolff, dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, 
#ktexteditor, hase, michaelh, ngraham, bruns, demsking, head7, kfunk, sars

D17932: Improvements to completion

[Milian Wolff]

mwolff added subscribers: brauch, kfunk, apol.
mwolff added a comment.


  In D17932#392060 , @thomassc wrote:
  
  > Thanks for reviewing. Regarding the question about which models would have 
an insensitive exact match, and which ones have sensitive exact matches:
  >
  > - An example for case-insensitive exact matches might be plain text, or a 
hypothetical case-insensitive programming language. For example for plain text, 
one might want to treat words like "Question" and "question" as exact matches, 
which will make the completion widget close itself when it shows one of them 
and the user types the other. This is the current behavior for the word 
completion in KWrite / Kate.
  > - An example for case-sensitive exact matches would be a case-sensitive 
programming language like C++. If the user typed "m_var" but the variable is 
actually called "m_Var", then the completion widget should not hide itself, 
since it might still be useful to replace the typed text with the completion 
item that has different case.
  
  
  please put that information into the commit message - I think it's valuable 
to have
  
  overall, I think I'm in favor of getting this in - but I'd like to get input 
from others. @brauch , @kfunk, @apol what do you have to say to this?

INLINE COMMENTS

> thomassc wrote in katecompletionmodel.h:394
> I added asserts to the setters. One should be aware then though that it 
> restricts the order in which these must be called. For example, if both 
> settings are case-insensitive at first and both should be changed to 
> case-sensitive, then the exact-match-sensitivity must be changed before the 
> match-sensitivity. Alternatively, one could make a single setter function 
> that changes both properties.

either that, or handle it gracefully (with a warning?) wherever it's used?

REPOSITORY
  R39 KTextEditor

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

To: thomassc, #ktexteditor, #kdevelop, mwolff
Cc: apol, kfunk, brauch, mwolff, cullmann, kwrite-devel, kde-frameworks-devel, 
hase, michaelh, ngraham, bruns, demsking, head7, sars, dhaumann

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

[Milian Wolff]

mwolff added inline comments.

INLINE COMMENTS

> mwolff wrote in textdocument.cpp:691
> can't you just add a slot here that removes the actions we added once the 
> menu is closed? that would fix this issue with way less code

and with slot I mean an local lambda that takes a copy of the actions list

REPOSITORY
  R32 KDevelop

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

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

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

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> textdocument.cpp:691
> -menu->addAction(action);
> -}
> -}

can't you just add a slot here that removes the actions we added once the menu 
is closed? that would fix this issue with way less code

REPOSITORY
  R32 KDevelop

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

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

D17932: Improvements to completion

[Milian Wolff]

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


  Hey there, sorry for the long delay. In general, I think your suggestions are 
very sane - most notably preferring exact case matches over fuzzy matches is a 
good thing to have!
  
  But I wonder: do we really need to make this configurable? Can't we just 
always do this? I.e. can you explain which models would have an insensitive 
exact match, and which ones have sensitive exact matches?
  
  Can you please submit the next patch iteration with context (I suggest you 
use `arc diff` for that)

INLINE COMMENTS

> katecompletionmodel.cpp:1554
>  
> -if(m_unimportant && !rhs.m_unimportant){
> +if (m_unimportant && !rhs.m_unimportant) {
>  return false;

unrelated whitespace changes should be fixed in separate commits

> katecompletionmodel.cpp:1575
> +
> +if( thisStartWithFilter && ! rhsStartsWithFilter ) {
> +return true;

here and below:remove space after `!`

> katecompletionmodel.cpp:1888
>  
> +QChar toLowerIfInsensitive(QChar c, Qt::CaseSensitivity caseSensitive) 
> +{

static or anon namespace

> katecompletionmodel.cpp:2026
>  
>  if (matchCompletion && match.length() == m_nameColumn.length()) {
> +if (model->matchCaseSensitivity() == Qt::CaseInsensitive &&

maybe simplify this to:

if (matchCompletion && m_nameColumn.startsWith(match, 
model->exactMatchCaseSensitivity())) {

  matchCompletion = PerfectMatch;
  m_haveExactMatch = true;

}

> katecompletionmodel.cpp:2029
> +model->exactMatchCaseSensitivity() == Qt::CaseSensitive &&
> +! m_nameColumn.startsWith(match, Qt::CaseSensitive)) {
> +return matchCompletion;

remove whitespace after !

> katecompletionmodel.h:394
> +Qt::CaseSensitivity m_matchCaseSensitivity = Qt::CaseInsensitive;
> +Qt::CaseSensitivity m_exactMatchCaseSensitivity = Qt::CaseInsensitive;  
> // Must be equal to or stricter than m_matchCaseSensitivity.
> +

should this comment be asserted in the setters or is it handled gracefully in 
the logic below?

REPOSITORY
  R39 KTextEditor

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

To: thomassc, #ktexteditor, #kdevelop, mwolff
Cc: mwolff, cullmann, kwrite-devel, kde-frameworks-devel, hase, michaelh, 
ngraham, bruns, demsking, head7, kfunk, sars, dhaumann

D17241: Disable highlighting for lines longer than 1024 characters.

[Milian Wolff]

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


  what sven said, we should also remove the code to disable highlighting 
altogether when the line limit is reached, no?

INLINE COMMENTS

> katerenderer.cpp:387
>  
> +if (textLine->length() > 1024 && !selectionsOnly) {
> +return newHighlight;

put the 1024 into a constant and use it here and below such that we ensure the 
number stays in sync

also, don't we have a setting for the line length limit? shouldn't that be used 
instead here?

> katerenderer.cpp:400
> +const QVector  = 
> textLine->attributesList();
> +for (int i = 0; i < al.count(); ++i) {
> +if (al[i].length > 0 && al[i].attributeValue > 0) {

this style-change should be submitted independently of this code review

REPOSITORY
  R39 KTextEditor

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

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

D13515: Remove KNS::Engine d-pointer hack

[Milian Wolff]

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


  this is binary compatible from what I can see

REPOSITORY
  R304 KNewStuff

BRANCH
  master

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

To: apol, #frameworks, leinir, mwolff
Cc: mwolff, kde-frameworks-devel, michaelh, ngraham, bruns

D13365: Fixed the cursor(caret) width in kate

[Milian Wolff]

mwolff added a comment.


  @shubham looks like you are a victim of 
https://www.kalzumeus.com/2010/06/17/falsehoods-programmers-believe-about-names/
 right? :D

REPOSITORY
  R39 KTextEditor

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

To: shubham, #ktexteditor, brauch, cullmann
Cc: mwolff, cullmann, ngraham, brauch, kwrite-devel, kde-frameworks-devel, 
michaelh, kevinapavew, bruns, demsking, sars, dhaumann

Re: kdesrc-build: cmake should take local (instead of system-wide) cmake modules

[Milian Wolff]

On Dienstag, 8. Mai 2018 22:40:39 CEST gregor.mi.sw wrote:
> Hello,
> 
> I have a question regarding kdesrc-build and CMake.
> 
> I setup the build environment variables and ran kdesrc-build and got a
> compiler error kinfocenter.
> 
> I investigated
> /home/gregor/kde/src/build/kde/workspace/kinfocenter/CMakeCache.txt and
> found the following lines
> 
>  //The directory containing a CMake configuration file for KF5Service.
>  KF5Service_DIR:PATH=/usr/lib64/cmake/KF5Service
> 
>  //The directory containing a CMake configuration file for KF5Solid.
>  KF5Solid_DIR:PATH=/usr/lib64/cmake/KF5Solid
> 
>  //The directory containing a CMake configuration file for KF5Wayland.
>  KF5Wayland_DIR:PATH=/usr/lib64/cmake/KF5Wayland
> 
> The directories of the needed KF5 frameworks point to the system wide
> installed ones.
> 
> I removed the system-wide devel package for solid (because it caused the
> compiler error) and ran kdesrc-build again:
> 
>  //The directory containing a CMake configuration file for KF5Service.
>  KF5Service_DIR:PATH=/usr/lib64/cmake/KF5Service
> 
>  //The directory containing a CMake configuration file for KF5Solid.
>  KF5Solid_DIR:PATH=/home/gregor/kde/usr/lib64/cmake/KF5Solid
> 
>  //The directory containing a CMake configuration file for KF5Wayland.
>  KF5Wayland_DIR:PATH=/usr/lib64/cmake/KF5Wayland
> 
> Now it shows the correct (local) path for solid (but not the other ones). Is
> there an environment variable or something I have to set to tell Cmake to
> always look for local modules first?

Try CMAKE_PREFIX_PATH. See e.g. this old blog post on the matter:

https://blogs.kde.org/2008/12/12/how-get-cmake-find-what-you-want-it

Cheers

-- 
Milian Wolff
m...@milianw.de
http://milianw.de

signature.asc
Description: This is a digitally signed message part.

D12756: [KDateTable] Use a more visible red color

[Milian Wolff]

mwolff added a comment.


  Let's try to fix the bug for real, instead of implementing half-baked 
workarounds that only work for the default configurations.
  
  Alternative ideas: add setters for the two possible cell text colors, then 
somehow set the KColorScheme color from the outside.
  
  We really should upstream more of KColorScheme to Qt/QPalette...

REPOSITORY
  R236 KWidgetsAddons

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

To: ngraham, #frameworks, cfeck
Cc: mwolff, apol, michaelh, ngraham, bruns

D12756: [KDateTable] Use a more visible red color

[Milian Wolff]

mwolff added a comment.


  Maybe instead use the HighlightText QPalette color? Hardcoding red may work 
for the two styles you present, but I could just set the view background to red 
and the text becomes unusable.

REPOSITORY
  R236 KWidgetsAddons

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

To: ngraham, #frameworks, cfeck
Cc: mwolff, apol, michaelh, ngraham, bruns

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Milian Wolff]

mwolff accepted this revision.
mwolff added a comment.


  OK, cool! That clearly shows that this patch _is_ valuable: Before we have 
~6% CPU cycle cost, now it's down to 1.5% (inclusively). This is a significant 
reduction, so I'm all for it.
  
  But note once again the stark difference in these numbers vs. what valgrind 
reports. Please, try to use `perf record --call-graph dwarf` + hotspot 
flamegraphs more in favor of callgrind/kcachegrind. Note that you can even use 
hotspot's record page for some more features, most notably the off-CPU 
profiling. This can give you more food for potential optimizations (finding 
sleep time, I/O, lock contention, ...).
  
  Happy profiling!

REPOSITORY
  R241 KIO

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

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Milian Wolff]

mwolff added a comment.


  `perf record -g` produces unusable data files, since it relies on the frame 
pointer which is usually not available. Use `perf record --call-graph dwarf` 
instead. 
https://phabricator.kde.org/file/data/w4qogv4brtxlc5p5bnwr/PHID-FILE-q62giymcptudpl5m6bt3/kwrite_perf_after_25_dwarf_caller.png
 shows ~1.5% in notifyAboutRangeChange (inclusively). Is that before or after 
your patch here?

REPOSITORY
  R241 KIO

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

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Milian Wolff]

mwolff added a comment.


  Actually, no. Ignore what I said. The pictures you are showing are pretty 
meaningless. Did you run perf with `--call-graph dwarf`? Better look at the 
flamegraph and search for the function you are interested in 
(Kate::TextBuffer::notifyAboutRangeChange) or use the Caller/Callee view to get 
an aggregated view of your change.

REPOSITORY
  R241 KIO

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

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Milian Wolff]

mwolff added a comment.


  But the hotspot screenshot clearly shows that you are spending time on 
optimizing things that are barely noticeable. You have optimized a function 
that consumes 0.3% of the CPU cycles. It now consumes only ~0.15%, at the cost 
of slightly higher memory consumption.
  
  This is the real cost. The instruction numbers reported by callgrind paint a 
very different picture (22.6% vs. 5.5%). But that's not what actually matters, 
the cycle cost is way more important.
  
  Also note that hotspot shows inclusive costs. You should get used to it, most 
notably have a look at the flamegraph and get acquainted with that way to 
visualize profiling costs.

REPOSITORY
  R241 KIO

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

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Milian Wolff]

mwolff added a comment.


  Oh and again: please start using perf/hotspot instead of callgrind. Really, 
the performance numbers you get from callgrind are just *instructions*! It 
doesn't mean "65% of CPU". It means 65% of the instructions.
  
  You are doing such good work with profiling and optimizing software, it would 
be so much better if you'd use the correct tools too!

REPOSITORY
  R39 KTextEditor

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

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12511: optimization of KTextEditor::DocumentPrivate::views()

[Milian Wolff]

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


  lgtm in general, but codewise can be improved

INLINE COMMENTS

> katedocument.cpp:2835
>  m_views.insert(view, static_cast(view));
> +m_viewsCache = m_views.keys();
>  

just add the view, cf. the contains check above. no need to rebuild the full 
list here all the time

> katedocument.cpp:2852
>  m_views.remove(view);
> +m_viewsCache = m_views.keys();
>  

dito, just remove

REPOSITORY
  R39 KTextEditor

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

To: jtamate, #kate, cullmann, mwolff
Cc: mwolff, cullmann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, sars, dhaumann

D12016: [ktexteditor] much faster positionFromCursor

[Milian Wolff]

mwolff added a comment.


  lgtm, @cullmann ?
  
  regarding the accessibility interface, I agree that it's out of the scope of 
this patch. I'd say let's keep it like that for now...

REPOSITORY
  R39 KTextEditor

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

To: jtamate, #kate, cullmann, #frameworks
Cc: mwolff, brauch, cullmann, #frameworks, michaelh, kevinapavew, ngraham, 
bruns, demsking, sars, dhaumann

D11902: Add highlighting for GDB command listings and gdbinit files

[Milian Wolff]

This revision was automatically updated to reflect the committed changes.
Closed by commit R216:88288f834175: Add highlighting for GDB command listings 
and gdbinit files (authored by mwolff).

REPOSITORY
  R216 Syntax Highlighting

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11902?vs=31207=32262

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

AFFECTED FILES
  autotests/folding/highlight.bt.fold
  autotests/folding/highlight.gdb.fold
  autotests/folding/highlight.gdbinit.fold
  autotests/html/highlight.bt.html
  autotests/html/highlight.gdb.html
  autotests/html/highlight.gdbinit.html
  autotests/input/highlight.bt
  autotests/input/highlight.gdb
  autotests/input/highlight.gdbinit
  autotests/reference/highlight.bt.ref
  autotests/reference/highlight.gdb.ref
  autotests/reference/highlight.gdbinit.ref
  data/syntax/gdb-bt.xml
  data/syntax/gdb.xml
  data/syntax/gdbinit.xml

To: mwolff, vkrause, dhaumann, #kate, cullmann
Cc: cullmann, #frameworks, michaelh, ngraham, bruns

D11902: Add highlighting for GDB command listings and gdbinit files

[Milian Wolff]

mwolff added a comment.


  Ping? Or can I just commit?

REPOSITORY
  R216 Syntax Highlighting

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

To: mwolff, vkrause, dhaumann, #kate
Cc: #frameworks, michaelh, ngraham, bruns

D12016: [ktexteditor] much faster positionFromCursor

[Milian Wolff]

mwolff added a comment.


  some nits, otherwise lgtm assuming all tests pass
  
  bonus points for a proper benchmark that shows the before/after gain in hard 
numbers

INLINE COMMENTS

> kateviewaccessible.h:191
>  {
> -int pos = 0;
> -for (int line = 0; line < cursor.line(); ++line) {
> -// length of the line plus newline
> -pos += view->view()->document()->line(line).size() + 1;
> +int pos = m_lastPosition;
> +const KTextEditor::Document *doc = view->view()->document();

I know the old code used int already, but shouldn't this better be a quint64 as 
it's a file offset?

> kateviewaccessible.h:192
> +int pos = m_lastPosition;
> +const KTextEditor::Document *doc = view->view()->document();
> +

const auto *doc =

> kateviewaccessible.h:204
> +m_lastView = view;
>  }
> +else {

style: join with next line

> kateviewaccessible.h:215
> +pos += cursor.line() - m_lastCursor.line();
> +}
> +else {

dito

> kateviewaccessible.h:231
>  private:
> +
>  inline KateViewInternal *view() const

unrelated change?

> kateviewaccessible.h:270
> +mutable KTextEditor::Cursor m_lastCursor;
> +// m_lastPosition stores the positionFromCursor, with the cursor always 
> in column 0
> +mutable int m_lastPosition;

maybe also mention that this is a file offset?

REPOSITORY
  R39 KTextEditor

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

To: jtamate, #kate, cullmann, #frameworks
Cc: mwolff, brauch, cullmann, #frameworks, michaelh, kevinapavew, ngraham, 
bruns, demsking, sars, dhaumann

D11811: avoid Asan runtime error: shift exponent -1 is negative

[Milian Wolff]

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


  thanks

REPOSITORY
  R39 KTextEditor

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

To: jtamate, #kate, #frameworks, mwolff
Cc: mwolff, brauch, michaelh, kevinapavew, ngraham, demsking, cullmann, sars, 
dhaumann

D11685: Implement single click on line number to select line of text

[Milian Wolff]

mwolff added a comment.


  the "also" in your commit message: can you split this commit into two parts, 
or is the feature addition also fixing the bug? Put differently: Could you 
first fix the bug, then add the feature, in separate commits?

INLINE COMMENTS

> kateviewhelpers.cpp:2018
>  m_lastClickedLine = t.line();
> -if (positionToArea(e->pos()) != IconBorder && 
> positionToArea(e->pos()) != AnnotationBorder) {
> +auto area = positionToArea(e->pos());
> +if (area != IconBorder && area != AnnotationBorder) {

const

> kateviewhelpers.cpp:2019
> +auto area = positionToArea(e->pos());
> +if (area != IconBorder && area != AnnotationBorder) {
> +auto pos = QPoint(0, e->y());

can you comment this code, why exclude these areas?

> kateviewhelpers.cpp:2183
>  const KateTextLayout  = m_viewInternal->yToKateTextLayout(e->y());
> +auto area = positionToArea(e->pos());
>  if (t.isValid()) {

const

> kateviewinternal.cpp:2787
> +placeCursor(pos);
> +m_possibleTripleClick = true;
> +}

dito comment, what has triple click to do with *begin* of select line? Is this 
b/c an actual triple click would select the line?

REPOSITORY
  R39 KTextEditor

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

To: rkron, #frameworks, #kate, #ktexteditor, ngraham, cullmann
Cc: mwolff, richardbowen, ngraham, #ktexteditor, #kate, #frameworks, michaelh, 
kevinapavew, demsking, cullmann, sars, dhaumann