Re: KDE Gear projects with failing CI (master) (25 June 2024)

[Sune Vuorela]

On 2024-06-26, Ben Cooksley  wrote:
> Possible fallout of the Windows image being rebuilt, except libssh hasn't
> changed since 2023 in Craft, so this must be due to Qt changes.
> Pretty sure we've seen this type of error before though? (curiously,
> qplatformdefines.h should include sys/types.h and therefore solve this
> issue)

I think libssh is quite new added to the image ?

anyways. I did a MR and also fixed a .. weirdness on the way. Reviews
welcome.

/Sune

Re: Incubation Request: Kiview

[Sune Vuorela]

On 2024-04-28, Méven  wrote:
> The immediate goal with this application is to fill this request feature
> for dolphin :
> https://bugs.kde.org/show_bug.cgi?id=272539
> And we can imagine reusing it in many other places potentially.

I just opened 'DocumentViewer' class and spent 5 minutes and got a bit
scared.

"instant" preview?

Launching a background libreoffice?
Doing weird command line parsing of libreoffice? and bash pipe grep ?
Then throwing a generated pdf at qtwebengine and hoping the best?

There is a lot of std::tsring to qstring and back again conversions

There are plenty of std::string deepcopies.

Who deletes ConversionThread ?

If this class is in any way representative of the code quality of the
app, I really think we should reconsider.

If this class is not representative, then it definitely should be
architecturally re-done.

/Sune

Re: KDE Builder: request for review

[Sune Vuorela]

On Mon, Apr 8, 2024 at 10:05 AM  wrote:
> and deprecating the kdesrc-build. That way we can move forward with new tool.

I don't think reviewing or not of kde-builder should have any effect on
kdesrc-build.

I think also we should be slightly wary of changing a long-running 20y
old tool written by long time kde people who are still around with a
brand new tool written by a brand new contributor.
I wonder if it will have the same shelf life as the ruby version that
was done some years ago and abandoned.

/Sune

Re: Should we stop distributing source tarballs?

[Sune Vuorela]

On 2024-04-04, Ben Cooksley  wrote:
>> I do also think it is nice if we get someone else to verify that the
>> tarball we ship actually matches the tag. I think some people in
>> distributions have already started looking into verifying that.
>>
>
> Hopefully they'll be gentle with tooling that does this?

I have only seen 'starting to look into it', not actually yet figuring
out what to do with it.

But as an approximation, I would expect

'does the tarball content match the tag?'

'can we easily and automatically explain the difference?'

'does it use autotools?'

'can autogenerated docs and stuff be moved to build time instead?'

and other such things that might flag it differently and probably for
manual inspections.

I think we have stopped injecting translations at tarball creation time
and our tarballs should actually match the git tags?

/Sune

Re: Should we stop distributing source tarballs?

[Sune Vuorela]

On 2024-04-03, Albert Vaca Cintora  wrote:
> What's the advantage of providing tarballs?

I do think there is an advantage in being able to verify that the soure
tarball is the same across distributions. Using a checksum on the
tarball is an easy way of doing it. Different git invocations for git
archive, different tar options and so on can create different checksums
for the same content.

I do also think it is nice if we get someone else to verify that the
tarball we ship actually matches the tag. I think some people in
distributions have already started looking into verifying that.

Also, git tags can be moved.

/Sune

Re: resvg

[Sune Vuorela]

On 2024-03-14, Igor Mironchik  wrote:
> Hello,
>
> What do you think about https://github.com/RazrFalcon/resvg in case of 
> processing and rendering SVGs?
>
> Do you have any plans to have this in Craft?

With the current revitalization of QtSvg, I kind of think we should work
harder with that rather than try to replace it.
It is after all hooked in quite deep in our stuff already, so most of
our svg's needs to be compatible with QtSvg anyways.

/Sune

Re: KDE Gear projects with failing CI (release/24.02) (28 February 2024)

[Sune Vuorela]

On 2024-02-28, Albert Astals Cid  wrote:
> konversation - 1st week
>  * https://invent.kde.org/network/konversation/-/pipelines/616933
>   * cmake fails to find dbus on Linux CI

This seems like intentional sillyness from the konversation developers.
Let's fail at build time for runtime components that are used in fringe
usecases.
My suggestion is to revert the offending commits.

/Sune

Re: KDE Gear projects with failing CI (master) (28 February 2024)

[Sune Vuorela]

On 2024-02-28, Albert Astals Cid  wrote:
>
> ark - 1st week
>  * https://invent.kde.org/utilities/ark/-/pipelines/616910
>   * flatpak fails complaining about KF6BreezeIcons

I think this is due to an old flatpak environment 
https://invent.kde.org/frameworks/breeze-icons/-/commit/a4970ce40ec43c0a1db418aa05ee2853497a0c14

So will probably be fixed together with the rest.

/Sune

Re: Post-MegaRelease projects

[Sune Vuorela]

On 2024-02-22, Nate Graham  wrote:
> I've started pondering post-megarelease projects. We've spent so long on 
> porting and bugfixing that I think it might be useful to shift gears to 
> feature work, and I'd like to brainstorm potential large-scale projects 
> and gauge the level of interest in putting resources into them soon.

A bit more from the devops end that I'd love to see people tackle:

 - Ensure frameworks and app unit tests interacting with windows can run
   on Windows.
   More details: The following fails on our windows CI
   https://invent.kde.org/sune/windows-test-thingie/-/blob/master/main.cpp
   I find it weird that we are spending resources on putting things in
   the windows store and making apps available on windows, but we can't
   actually have passing tests in our CI.

 - Find a way to run unit tests on android CI.

 - Make autotests guarding on all our CI's.

 - Clazy and clang-tidy and cppcheck on all our repositories in CI

 /Sune

Re: KDE Gear projects with failing CI (master) (20 February 2024)

[Sune Vuorela]

On 2024-02-21, Sune Vuorela  wrote:
> On 2024-02-21, Ben Cooksley  wrote:
>>> ark - 3rd week
>>>  * https://invent.kde.org/utilities/ark/-/pipelines/611869
>>>   * tests fail on FreeBSD
>>>
>>
>> Wonder if the tests properly handle umask being set?
>
> I agree that the issue is likely here

Force umask:

https://invent.kde.org/utilities/ark/-/merge_requests/234

/Sune

Re: KDE Gear projects with failing CI (master) (20 February 2024)

[Sune Vuorela]

On 2024-02-21, Ben Cooksley  wrote:
>> ark - 3rd week
>>  * https://invent.kde.org/utilities/ark/-/pipelines/611869
>>   * tests fail on FreeBSD
>>
>
> Wonder if the tests properly handle umask being set?

I agree that the issue is likely here

/Sune

Re: revisiting the sycoca

[Sune Vuorela]

On 2024-02-13, Harald Sitter  wrote:
> Do you have a testing plan in mind?

Not a dedicated one, no. But assuming your analysis is mostly correct,
something along the lines of 

 - What's the plasma startup performance with and without sycoca on a
   clean user
   - If much slower, try isolate which of the 3 parts are responsible
 - What's the startup performance if a user has done modifications like
   menu changes and user-environment installed apps/desktop files [a]
 - What happens when a user adds their own mimetype additions ? That
   could also be loading a folder with such files in dolphin or a 'save
   /open'-dialog


I'm assuming this is mostly disk intensive operations, so a slow disk
(spinning metal) is probably a good idea for that.


I wonder if we should also try to get David Faure to look at this
thread.

/Sune

[a] Isn't this even more likely in the world of flappacks and snacks and
appimages and snaps and flatpaks and such

Re: revisiting the sycoca

[Sune Vuorela]

On 2024-02-08, Harald Sitter  wrote:
> It occurs to me that we should ponder sycoca a bit.
>
> Currently the sycoca contains 3 types of caches:

At some point I remember David Faure, iirc as part of his QMimeType
work, removed part of sycoca, but left the remaining bits as a per 
user cache *because* we allow the user to modify things.

I don't know how much of it is still relevant, but I do think we should
be testing it, both on modern ssd systems, but given our occasional
marketing drive about keeping older systems running, especially also 
on older systems with a spinning metal disk, rather than a bit handwavy
"This is probably going to be fine"

/Sune

Re: KDE Frameworks with failing CI (master) (29 January 2024)

[Sune Vuorela]

On 2024-01-29, Albert Astals Cid  wrote:
> Bad news: 6 repositories have started failing
>
> baloo:
> kconfig:
> kcontacts
> kfilemetadata:
> ki18n:
> threadweaver:
>   * FreeBSD tests are failing

I haven't studied these, and don't know if they are frequent or
occasional failures. I have seen, after the fbsd builder changes, that
test execution times have gone up 20-50%. If it is big tests that is
already close to the limit, then that might be the reason.

Or for others with occasional timeout tests on freebsd.

/Sune

What can we expect from our developers

[Sune Vuorela]

On 2024-01-29, Jonathan Riddell  wrote:
> This sort of comment  makes me really sad. The All About the Apps goal,
> which in principle is still ongoing, was an attempt to get KDE developers
> to realise it was important not just to write apps but to actually make
> them available to users, I find it astonishing how we still don't have a
> culture where making our apps available to users is part of our
> responsibility.  There's teams in KDE to help with all these formats.
> Sorry to complain here as the issue is larger than just this one app but
> it's so sad that nobody within KDE wants to help get users using our
> software directly.

I think this is taking it too far. I think the goal is more about not
getting in the way of people who wants to do this.

We need to draw a line somewhere about what we expect from *all* of our
developers.

I don't think that we can expect all of our developers to be great at
 * c++
 * qtquick
 * cmake
 * windows weirdness
 * linux weirdness
 * freebsd weirdness
 * osx weirdness
 * android weirdness
 * packaging flatpaks, appimages, snaps, debs, rpm, .. for linux
 * making osx installers
 * making apk's
 * making windows installers
Beside the domain of the application.

Yet it is kind of what we are doing with having gating CI setups for
many of these, and adding more.

I'm quite a seasoned developer and I know I can't care for all of that.
I also don't have the time to care for all of these.

We also don't have extra manpower in the teams that knows about these to
help everywhere.

We might have a goal about this, but this is just far too many thing to
be good at everything.

I don't think we should shame individual developers for also realizing
this.

But where should we draw the line ?

/Sune

Re: Can my application, which contains dirty code, become an official kde application?

[Sune Vuorela]

On 2024-01-17, Danilo Agostini  wrote:
> It allows the user, through the use of a keyboard shortcut or a dolphin
> servicemenu, to have a quick preview of the files that are shown in the
> folder without having to open the default application.
>
> Similar applications are Gnome Sushi and Quick Look (Mac os).

Isn't this in the similar realm as what we can do with thumbnailers
and/or kparts ?

> Technical Limitations/Defects:
>
> 1) The way it integrates with dolphin is not clean due to dolphin's
> limitations: I currently use dbus to copy the path of the selected file

I'd suggest to fix whatever limitations there is in Dolphin to not  have
to do workarounds.

> 2) The preview of some files (odt,doc,docx,xlsx,etc) is obtained by
> converting the documents to pdf using the "libreoffice --headless
> --nolockcheck --norestore --convert-to pdf" command. This obviously
> requires libreoffice installed on the system and the conversion may fail/be
> slow in some cases.

I don't think this as such is a bad idea except maybe ... doesn't this
negate the whatever speedup you are trying to do with a 'quick preview'
instead of launching the app when you now actually launch the app
headless in the background ?


I just browsed around in the code and I do think there is quite some
cleanups to do. I'm sure cppcheck, clazy, clang-tidy and other static
analyzers can find a lot.

oh. Don't use rand(). And consider using something to create a temporary
working directory rather than work directly in tmpdir. 

Some members are m_ prefixed. others aren't. Some member vars shouldn't
be member vars. 
Consider using much more const refs or views.

I'm confused about when you use std::string and QString. It seems to be
a bit random which one you pull out of the tool chest.
And std::thread and QThread.

Why are you using std::filesystem::__cxx11::path ? (Especially the
__cxx11 part)

Also there seems to be a pattern of 
|QString foo;
|foo = someFunction();
Just having 
|QString foo = someFunction();
Is normally easier to work with.


Also note that we are about to move everything to Qt6/KF6; This seems to
be stuck on quite ancient frameworks and qt versions.

/Sune

Re: KDE Gear projects with failing CI (release/24.02) (Branching edition)

[Sune Vuorela]

On 2024-01-12, Sune Vuorela  wrote:
> On 2024-01-12, Albert Astals Cid  wrote:
>> okular:
>>  * https://invent.kde.org/graphics/okular/-/pipelines/580655
>>   * It's trying to find packages in a wrong branch
>
> On it. Will report a MR for others with likely similar issues.

https://invent.kde.org/graphics/okular/-/merge_requests/908

fixed it in master and release branch

/Sune

Re: KDE Gear projects with failing CI (release/24.02) (Branching edition)

[Sune Vuorela]

On 2024-01-12, Albert Astals Cid  wrote:
> okular:
>  * https://invent.kde.org/graphics/okular/-/pipelines/580655
>   * It's trying to find packages in a wrong branch

On it. Will report a MR for others with likely similar issues.

/Sune

Re: plugins-to-plugins and mimetypes and json

[Sune Vuorela]

On 2024-01-05, Sune Vuorela  wrote:
> Or is this a feature that has just been accidentally lost and thus we
> accidentally killed advanced usage of kparts ?

https://invent.kde.org/frameworks/kparts/-/merge_requests/82

and 

https://invent.kde.org/graphics/okular/-/merge_requests/902

makes this work for kparts.

Do we have other plugin types where we might want to add plugins?

Do we have other places than okularpart where we have
plugins-in-plugins?

/Sune

plugins-to-plugins and mimetypes and json

[Sune Vuorela]

Hi wise peoples

During the switch to KF6, all plugins seems to now be required to use
the json based plugin approach, which has quite many advantages, like
"data embedded into the binary and no change of getting out of sync"

Unfortunately it also seems to have at least one disadvantage. How do I
extend plugins with plugins and then let my "main" plugin tell the world
which mimetypes it supports. (3rdparties can also provide these
plugins)

Less theoretical and more practical:

the Okular KPart can show documents. But by itself, it can't do
anything. It has plugins (called generators).

If you build and install the pdf generator, the kpart now supports viewing
pdf documents. If you install the dvju generator, the kpart now supports
dvju documents.

How do I tell e.g. konqueror that the okular kpart supports  ?

Or is this a feature that has just been accidentally lost and thus we
accidentally killed advanced usage of kparts ?

/Sune

Re: KF6 porting. KSelectAction::triggered signal

[Sune Vuorela]

On 2024-01-04, Sune Vuorela  wrote:
> On 2024-01-03, Albert Astals Cid  wrote:
>> scripty has everything checked out so if you tell me what to grep for, I can 
>> run a query on it.
>
> I got access to the lxr machine and has been grep'ing there instead, and
> I only found one maybe-issue that I need to investigate

Patch submitted.

I was grepping for KSelectAction::triggered and then going thru the
occurences and ignoring those that were in Qt5 ifdeffery.

/Sune

Re: KF6 porting. KSelectAction::triggered signal

[Sune Vuorela]

On 2024-01-03, Albert Astals Cid  wrote:
> scripty has everything checked out so if you tell me what to grep for, I can 
> run a query on it.

I got access to the lxr machine and has been grep'ing there instead, and
I only found one maybe-issue that I need to investigate

/Sune

KF6 porting. KSelectAction::triggered signal

[Sune Vuorela]

Hi

I just - during KF6 porting of an app - fall in an api trap:

Original code:

| connect(d->aZoom, QOverload::of(::triggered),
| this, ::slotZoom);

The wrong porting code:

| connect(d->aZoom, ::triggered,  this, ::slotZoom);

The gotcha is that the QAction* overload, while with the same name, is
emitted under different rules than the only remaining triggered function
(from QAction)

The correct porting would have been:

| connect(d->aZoom, ::actionTriggered, this,
| ::slotZoom);


So, just a heads up to whomever might hit this as well. (I wonder if one
has enough sources checked out to be able to grep this to see what might
be relevant.

/Sune

T11542: Remove KHTML

[Sune Vuorela]

svuorela closed subtask T11539: Port Okular away from KHTML as 
Resolved.

TASK DETAIL
  https://phabricator.kde.org/T11542

To: svuorela
Cc: cordlandwehr, ngraham, #konqueror, #plasma, #okular, #kde_applications, 
#frameworks, knauss, cpmontanari, ghost62, hannahk, davidre, GB_2, ahmadsamir, 
kpiwowarski, usta, asturmlechner, jucato, cullmann, vkrause

Re: QML: a packagers nightmare. Assistance please.

[Sune Vuorela]

On 2023-11-08, Kai Uwe Broulik  wrote:
> Hi,
>
> that WorkspaceComponents is used for a ShadowedLabel which is literally 
> one QML file with a Label and a DropShadow. KWin could just not use that 
> (and build its own) and we’d resolve the issue.

It sounds like either moving that component one level "up" in the
hierachy or this suggestion or something else would resolve the issue.
And in a nice way.

/Sune

Re: QML: a packagers nightmare. Assistance please.

[Sune Vuorela]

On 2023-11-08, Nicolas Fella  wrote:
> Furthermore, kwin is not a framework, it's part of Plasma, and
> dependencies between Plasma components are generally fine. That there is
> a circular dependency between plasma-workspace and kwin is not good and
> should probably be addressed somehow, but the severity is somewhat lower
> given the cycle doesn't exist at built time.

Though given packagers are being heavily suggested, and several has
already, to write tools to help atuomatically track QML dependencies to
ensure no depends: is left behind, cycles like these makes it impossible
to actually do so.

Either we should retract that suggestion and live with more broken QML
dependencies in distributions until users randomly fix it, or we should
try harder to not have such cycles, and when we see them be quick to fix
them.

/Sune

Re: Breeze and ECM are incompatible for installing icons

[Sune Vuorela]

On 2023-11-03, David Jarvie  wrote:
> I wanted to provide an icon that is visually compatible with the Breeze 
> theme, and Breeze doesn't supply it.

Did you consider submitting it to breeze ?

I have heard they don't bite that much ...

/Sune

Re: KDE Review: Hash-o-Matic

[Sune Vuorela]

On 2023-10-01, Carl Schwan  wrote:
> Hello,
>
> I started writting a small application to generate and compare files with 
> their 
> checksum two years. I piked it up again recently and I think this is now 
> ready 
> for a kde review.

Even two years ago, checking stuff with md5 was not a good idea, unless
just for checking for transfer errors.

But maybe add a sha3 variant instead?

/Sune

Re: kio-extras and the KF5/KF6 period

[Sune Vuorela]

On 2023-06-20, David Redondo  wrote:
> Harald and I prototyped another solution to build a Qt
>  5 and Qt 6 version out of the same repo and employed it on
> plasma-integration: https://invent.kde.org/plasma/plasma-integration/-/
> merge_requests/91

Did I miss something or is this just branching but without having git to
help us move stuff between versions?

/Sune

Re: kio-extras and the KF5/KF6 period

[Sune Vuorela]

On 2023-05-16, Albert Astals Cid  wrote:
> If we're going to support a period on which we ship both Kf5 and KF6 based 
> applications we need to:
>
> Make sure kf5 and kf6 are coinstallable.
>
> a) release two tarballs, one for each KF
> b) release one tarball that compiles both for kf5 and kf6 
> c) just release the kf6 tarball, stop releasing the kf5 tarball but ask 
> distributions to still install it

(wearing a quite stale debian hat)

If you go with c), please make sure that the "base" of the tarball
called something different. Not just version wise, but also the base
part of it.

I think most distributions can't handle having two tarballs with same
"name" but different versions.

I think if I got offered b), I'd implement a) myself by creating an
extra copy.

I guess c) ensures no more bugfixes to the kf5 version. 

I think I lean towards a).

/Sune

Re: Would Scandoc be somthing for Extragear?

[Sune Vuorela]

On 2022-11-10, Thomas Baumgart  wrote:
> I don't know, if you have thought of this when writing invoices. Include
> an EPC QR (https://de.wikipedia.org/wiki/EPC-QR-Code) on the invoice. I am
> thinking about adding a webcam interface so that KMyMoney can use it to
> read the QR code and use the data to fill out the form for online payments
> that KMyMoney already has. Or even use the PDF file as input an pull out
> the QR code. That would improve productivity I think. I know, a bit off
> topic here.

libprison and itinerary application might help you in this; they should
together have the features to both read barcodes from pdf's, from webcam
and generate barcodes.

/Sune

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

[Sune Vuorela]

On 2021-12-19, Nate Graham  wrote:
> For what it's worth, I don't think server-wise retracing is solely a 
> workaround for distro-specific issues. Even for distros that ship debug 
> symbols, I often find in my bug triaging that it can be a challenge to 
> get users to actually use them.

Server side retracing is king of orthogonal to this.

Server side retracing is taking the coredump (maybe minidump), shipping
it to a server and combining it with debuginfo from the original binary
build. But this would require the serverside retracer to be able to
consume debuginfo packages correctly for all (relevant?) distros and all
historic? versions.

The thing drkonqi does is it tries to do client side retracing and tries
to get the package manager to get the correct debuginfo packages
available. From a package manager perspective[*], that's often quite hard
to get right, and thus why it often does not give that good bug reports.
With debuginfod integration and distributions providing debuginfod, this
can become better.


But for both of them, it needs debuginfo available from somewhere. If
the distro don't provide those, then we, no matter what, are out of
luck.

/Sune

[*] Imagine user installs dolphin 5.4.3-1, experiences a crash 2 weeks
later, drkonqi tries to get package manager to install debug symbols,
but 5.4.4 has since come out. Unless the package manager systems keeps
debuginfo around much longer than the package it belongs to (which I
haven't heard anyone actually do), then this will not match.

Re: How are tier 1 framework qm translations supposed to be loaded?

[Sune Vuorela]

On 2021-01-26, Halla Rempt  wrote:
> Ah... I guess that explains it. GenericDataLocation is only useful in a linux 
> distribution packages setup, because it's the location that's shared with 
> other applications, so we patch ki18n to use AppDataLocation -- I guess we'll 
> have to patch ECM as well.

I'm not sure I would be opposed to having AppDataLocation also searched;
I don't think the runtime overhead would be significant.
I'm also not sure there would be significant security implications, and
it might work better on some platforms where there is only sometimes the
Generic location. (Iirc there are also platforms where Generic maps to
App)

/Sune

D29747: Deprecate AbstractBarcode::minimumSize() also for the compiler

[Sune Vuorela]

svuorela added a comment.


  I think we should wait a bit with formally deprecating. I like add the new, 
wait a bit, then deprecate.
  
  Also, while I might be a unconventional developer, but given this is a tier 1 
library, I'm often hand patching the required ECM version to a much lower 
version to test stuff when I know it will work. So I'd say. Yes. nice patch, 
but let's wait a month or two with submitting it.

REPOSITORY
  R280 Prison

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

To: kossebau, #frameworks, svuorela, vkrause
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28760: KSettings::Dialog: avoid duplicate entries due cascading $XDG_DATA_DIRS

[Sune Vuorela]

svuorela accepted this revision.
svuorela added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> dialog.cpp:317
>  const QStringList dirs = 
> QStandardPaths::locateAll(QStandardPaths::DataLocation, 
> QStringLiteral("ksettingsdialog"),  QStandardPaths::LocateDirectory);
> +QMap fileMap;
>  for (const QString  : dirs) {

I think I read once that whenever you used a ordered map over an unordered map, 
you need to justify it by talking to your manager about it. But that's also a 
bit from the bucket of nitpickery unless we are in a hot codepath.

> dialog.cpp:326
>  }
> +for (auto it = fileMap.constBegin(); it != fileMap.constEnd(); ++it) {
> +parseGroupFile(it.value());

nitpickery. but range based for and qAsConst on container?

REPOSITORY
  R295 KCMUtils

BRANCH
  fix_duplicates

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

To: dfaure, apol, broulik, davidedmundson, kossebau, svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[Sune Vuorela]

svuorela added a comment.


  I've been looking at it for a while, and after trying to decipher the long 
lambdas, which might have been better as named functions, I think it is, beside 
Kossebau's comments, fine.

REPOSITORY
  R241 KIO

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

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28765: KSettings::Dialog: add support for KPluginInfos without a KService

[Sune Vuorela]

svuorela accepted this revision.
svuorela added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> kcmoduleinfo.cpp:73
> +lib = pluginInfo.libraryPath();
> +keywords = 
> pluginInfo.property(QStringLiteral("Keywords")).toStringList();
>  }

At a later point, I*m not sure what the purpose is for these members are - but 
that's probably for another changeset.

> kcmoduleinfo.h:131
> + * @warning This will be null if this KCModuleInfo was created from a 
> KPluginInfo coming from KPluginMetaData.
> + * Prefer using pluginInfo() instead, which works for both kinds.
> + */

Can we mark it as deprecated?

REPOSITORY
  R295 KCMUtils

BRANCH
  master

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

To: dfaure, pino, broulik, mart, davidedmundson, ngraham, svuorela
Cc: svuorela, cblack, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

Re: Calindori in kdereview

[Sune Vuorela]

On 2020-04-12, Dimitris Kardarakos  wrote:
>
> Hello everyone,
>
> I'd like to let you know that the Calindori [1] application has been
> moved to kdereview.

I'm a bit curious about the name. Is it just a random butchering of
Calendar or does it actually have a meaning?

/Sune

D28030: Also expose the true minimum size to QML

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R280 Prison

BRANCH
  pending

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

To: vkrause, svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27952: Simplify minimum size handling

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R280 Prison

BRANCH
  pending

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

To: vkrause, svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27989: Add a new set of barcode size functions

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R280 Prison

BRANCH
  pending

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

To: vkrause, svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27909: Move barcode image scaling logic to AbstractBarcode

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R280 Prison

BRANCH
  pending

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

To: vkrause, svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27730: Add API to check whether a barcode is one- or two-dimensional

[Sune Vuorela]

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


  thanks.

REPOSITORY
  R280 Prison

BRANCH
  pending

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

To: vkrause, svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27730: Add API to check whether a barcode is one- or two-dimensional

[Sune Vuorela]

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


  I'm not sure I like the implementation. The current AbstractBarcode can also 
be implemented, at least theoretically, by software providing other barcodes 
that forever reason isn't available here. I have at least done that for some 
PoC ports of other things.
  
  I suggest instead having a 
  enum class BarcodeDimensions { 1d, 2d, 3d, 4d } ; 
  perhaps with better names, and then just a barcodeDimensions() method on 
AbstractBarcode that can be switched over.

REPOSITORY
  R280 Prison

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

To: vkrause, svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27727: Remove empty/unused private classes on internal types

[Sune Vuorela]

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


  These are leftovers from when all constructor was public.

REPOSITORY
  R280 Prison

BRANCH
  master

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

To: vkrause, svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D22419: Fix cant enter directory error on Android FTP servers

[Sune Vuorela]

svuorela added a comment.


  According to the RFC, the server should ignore case:
  
  > Upper and lower case alphabetic characters are to be treated
  >  identically. Thus any of the following may represent the retrieve
  > command:
  > 
  >   RETR Retr retr ReTr rETr
  
  I'm not sure how many buggy servers we should add workarounds for. One thing 
is the -a option which I'm not sure is actually standardized.

REPOSITORY
  R241 KIO

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

To: nxiss7, #frameworks
Cc: svuorela, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, 
bruns

Re: krecipes to unmaintained

[Sune Vuorela]

On 2019-05-09, Jonathan Riddell  wrote:
> I'd like to propose moving krecipes to unmaintained.  It still uses
> kdelibs4 and has had no feature commits since 2016.

I'd like to gentle promote KookBook here ... it has a krecipes converter
should anyone feel stuck with their data.

/Sune

Re: KDE service provider for handling arbitrary links (namely tel: links)

[Sune Vuorela]

On 2019-05-08, Hans-Peter Jansen  wrote:
> Since some time, special links like e.g. tel:+49... appear on the web, and 
> I've taught my browser (Firefox) to handle those as well. Now, I would like 
> integrate with kmail, chromium, ...

https://phabricator.kde.org/D18369

I think that patch helps enabling it for e.g. kmail.

/Sune

D20938: Add Mounts Backend

[Sune Vuorela]

svuorela added a comment.


  After reading the code a bit, I wonder
  
  - can't some of the parsing code be made testable (and tests added)
- This likely also requires to get rid of the global static.
  - rather than poll /proc/mounts every second, I was wondering if one of the 
filesystem notification things (QFileSystemWatcher, KDirWatch) could tell when 
to re-parse it

REPOSITORY
  R245 Solid

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

To: hallas, #frameworks, ngraham, elvisangelaccio, broulik, bruns
Cc: svuorela, nicolasfella, ivan, kde-frameworks-devel, michaelh, ngraham, bruns

D20939: Aztec: Fix padding if the last partial codeword is all one bits

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R280 Prison

BRANCH
  master

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

To: vkrause, svuorela, nicolasfella
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20312: get username from full name usernameFromFullname(QString fullname) -> QString

[Sune Vuorela]

svuorela added a comment.


  In D20312#445122 , @ahmedbilal 
wrote:
  
  >
  
  
  
  
  > @svuorela If system have user with same Real Name it would return the 
username of first user. I think, this function should be in Okular instead 
because there seems to be no use case where it would be used besides showing 
avatar for PDF's authors. Whats your comment on it.
  
  I think it is less important to get it perfect if it just goes into okular as 
part of the application. There it can be adapted and evolved slowly. But we 
should have it perfect if it should land in one of the frameworks. The API 
defined here is kind of cast in stone.
  
  /Sune

REPOSITORY
  R244 KCoreAddons

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

To: ahmedbilal, #frameworks, kde-frameworks-devel, svuorela
Cc: svuorela, aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D20312: get username from full name usernameFromFullname(QString fullname) -> QString

[Sune Vuorela]

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


  I think unit tests would be nice. Including unit tests documenting the 
behavior for multiple users with the same full name.
  
  My systems have several users named "Sune Vuorela"

INLINE COMMENTS

> kuser.h:412
> + */
> +static QString usernameFromFullname(QString fullname);
>  /**

What happens if multiple users have the same full name?

REPOSITORY
  R244 KCoreAddons

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

To: ahmedbilal, #frameworks, kde-frameworks-devel, svuorela
Cc: svuorela, aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D20149: Fix compilation

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R287 KImageFormats

BRANCH
  master

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

To: aacid, svuorela
Cc: svuorela, apol, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D20143: ora:kra: qstrcmp -> qstrncmp

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R287 KImageFormats

BRANCH
  arcpatch-D20143

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

To: aacid, svuorela
Cc: apol, pino, security-team, rempt, kde-frameworks-devel, michaelh, ngraham, 
bruns

D20145: Fix RGBHandler::canRead

[Sune Vuorela]

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


  I'm not sure why it as such is needed, but it isn't wrong.

REPOSITORY
  R287 KImageFormats

BRANCH
  master

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

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

D18369: Create tel: links for phone numbers

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R244 KCoreAddons

BRANCH
  master

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

To: vkrause, svuorela
Cc: apol, aacid, svuorela, nicolasfella, kde-frameworks-devel, michaelh, 
ngraham, bruns

D18369: Create tel: links for phone numbers

[Sune Vuorela]

svuorela added inline comments.

INLINE COMMENTS

> ktexttohtml.cpp:159
> +// this isn't 100% accurate, we filter stuff below that is too hard to 
> capture with a regexp
> +static const QRegularExpression telPattern(QStringLiteral(R"([+0](( |( 
> ?[/-] ?)?)\(?\d+\)?+){6,30})"));
> +const auto match = telPattern.match(mText, mPos, 
> QRegularExpression::NormalMatch, QRegularExpression::AnchoredMatchOption);

How are the thread safety of the KTextToHTML functions?

And does that match the thread safety of the QRegularExpression and the const 
methods in there?

REPOSITORY
  R244 KCoreAddons

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

To: vkrause
Cc: svuorela, nicolasfella, kde-frameworks-devel, michaelh, ngraham, bruns

D16561: Remove unused variable

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R495 Purpose Library

BRANCH
  master

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

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

D16567: Remove unused variable

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

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

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

D16569: Remove unused variable

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R304 KNewStuff

BRANCH
  master

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

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

D16568: Remove unused variables

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

BRANCH
  master

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

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

D16571: Remove unused variable

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R311 KWallet

BRANCH
  master

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

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

D16572: don't assign variable to itself

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R278 KWindowSystem

BRANCH
  master

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

To: aacid, svuorela
Cc: kde-frameworks-devel, mart, michaelh, ngraham, bruns

D16573: Remove unused variables

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R263 KXmlGui

BRANCH
  master

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

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

D16574: Remove unused variable

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R282 NetworkManagerQt

BRANCH
  master

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

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

D16565: Remove unused variable

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R237 KConfig

BRANCH
  master

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

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

D16137: Add an option to KConfigDialog to fit page contents horizontally

[Sune Vuorela]

svuorela added a comment.


  
  
  > Did you imagine something like what I changed in D16137#343060 
 (the latest change) ?
  
  I think Albert imagined something like that. It is at least what I imagined 
when I read his comments. I think I also like it better.

REPOSITORY
  R265 KConfigWidgets

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

To: kadabash
Cc: ngraham, svuorela, aacid, kde-frameworks-devel, michaelh, bruns

D16137: Add an option to KConfigDialog to fit page contents horizontally

[Sune Vuorela]

svuorela added a comment.


  from a source & binary compatibility point of view, this looks great.
  
  I guess we should maybe plan deprecating (and at kf6 time remove) the old 
ones ?  (But this is not an objection to the current change)
  
  I'm not a KConfigWidgets maintainer, so I will not right now say shipit. But 
if no objections by others until wed or thursday, consider this an accept.

REPOSITORY
  R265 KConfigWidgets

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

To: kadabash
Cc: svuorela, aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D15833: extend test coverage to all supported mimetypes for taglibextractor

[Sune Vuorela]

svuorela added a comment.


  What's the origin/license of these files ? How are they created ?

REPOSITORY
  R286 KFileMetaData

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

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

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Sune Vuorela]

svuorela added inline comments.

INLINE COMMENTS

> astippich wrote in taglibwritertest.cpp:66
> To be sure I understand correctly, using stringSuffix.toUTF8() is what you 
> would like to see here?

Something along those lines, yes please, to compare with.

REPOSITORY
  R286 KFileMetaData

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

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

D15715: remove own implementation of QString to TString conversion for taglibwriter

[Sune Vuorela]

svuorela added a comment.


  In D15715#330743 , @astippich 
wrote:
  
  > since I'm still unsure about those things: the q2t function was not 
declared static, but was never exported. It is still safe to remove, right?
  
  
  Yes. not exported (and not declared in a header file). Safe.

REPOSITORY
  R286 KFileMetaData

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

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

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Sune Vuorela]

svuorela added inline comments.

INLINE COMMENTS

> bruns wrote in taglibwritertest.cpp:66
> ?
> The file/tags are written inside this test.

yeah. given you write and read it, if somehow it gets encoded e.g. as 
iso-8859-15 rather than utf8, the euro sign would be encoded as 0xa4 rather 
than 0x20ac.

As you write and read it in the same sequence, there is a possibiliyt for this 
to pass when it shouldn't.

Unfortunately roundtripping the files with bad editors can make this happen. 
Especially on windows.

REPOSITORY
  R286 KFileMetaData

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

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

D15714: add a string suffix to test data and use for unicode testing of taglibwriter

[Sune Vuorela]

svuorela added inline comments.

INLINE COMMENTS

> taglibwritertest.cpp:66
> +QCOMPARE(extractedGenre, QString(QStringLiteral("Genre1") + 
> stringSuffix));
> +QCOMPARE(extractedComment, QString(QStringLiteral("Comment1") + 
> stringSuffix));
>  

I suggest checking that the last bytes of all these extracted things is the 
actual actual utf8 bytes, so that if someone compiles or saves this file in a 
broken encoding that the testing doesn't fail.

> taglibwritertest.cpp:75
>  QTest::addColumn("mimeType");
> +QTest::addColumn("stringSuffix");
>  

Given stringSuffix is the same for all tests, is it needed to have as a test 
data thing ?

REPOSITORY
  R286 KFileMetaData

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

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

D15704: increase test coverage of taglibwriter

[Sune Vuorela]

svuorela added a comment.


  Looks good to me. A nice and simple way to drastically increase test coverage.

INLINE COMMENTS

> taglibwriter.cpp:22
>  {
>  QStringList types = {
>  QStringLiteral("audio/mpeg"),

Unrelated. but consider making this static ?

REPOSITORY
  R286 KFileMetaData

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

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

D15615: bump required taglib version

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R286 KFileMetaData

BRANCH
  bumb_taglib_version

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

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

D15614: remove usage of own TString to QString conversion function

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R286 KFileMetaData

BRANCH
  taglib_extractor_string_conversion

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

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

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Sune Vuorela]

svuorela added a comment.


  @fvogt mostly a unit test that ensures that all these 3 codepaths in slotData 
works. I'm not sure that the current unit tests ensures that. I might be wrong 
though.

REPOSITORY
  R241 KIO

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

To: fvogt, #frameworks, elvisangelaccio, dfaure, svuorela
Cc: svuorela, ngraham, bruns, kde-frameworks-devel, michaelh

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Sune Vuorela]

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


  Though unit tests would be nice. It looks like something that quite easy 
could break if next author is not careful.

REPOSITORY
  R241 KIO

BRANCH
  master

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

To: fvogt, #frameworks, elvisangelaccio, dfaure, svuorela
Cc: svuorela, ngraham, bruns, kde-frameworks-devel, michaelh

D15526: Fix rendering glitches caused by rounding errors in Code 128

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R280 Prison

BRANCH
  master

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

To: vkrause, svuorela
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15527: Remove assumption about the barcode aspect ratio from the QML integration

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R280 Prison

BRANCH
  master

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

To: vkrause, svuorela
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15511: Add support for Code 128 barcodes

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R280 Prison

BRANCH
  master

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

To: vkrause, svuorela
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15511: Add support for Code 128 barcodes

[Sune Vuorela]

svuorela added inline comments.

INLINE COMMENTS

> CMakeLists.txt:26
> +)
> +# qt5_add_resources(aztecbarcodetest_srcs aztec/aztec.qrc)
> +

copy/paste error ?

> code128barcodetest.cpp:111
> +qDebug() << "Expected:" << output;
> +}
> +QCOMPARE(v, output);

doesn't QCOMPARE automatically output both ?

REPOSITORY
  R280 Prison

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

To: vkrause, svuorela
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15364: Plasma style: Remove hardcoded textFormat

[Sune Vuorela]

svuorela added a comment.


  I think it is undesireable to change the default, because all users are now 
used to having it PlainText, so they are now dumping 
things-that-looks-like-html into it and expecting it to be plaintext.
  
  so all users of Label that don't have textFormat set now needs to go add this 
line to their files.
  
  I also remember some discussions about the default to AutoText might have 
been wrong on Qt Text things (items and widgets) but can't be changed because 
people rely on the defaults.
  
  So please, no.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: jbbgameich, #plasma
Cc: svuorela, ngraham, kde-frameworks-devel, lnj, michaelh, bruns

D14529: Android: Make sure Qm translations get loaded

[Sune Vuorela]

svuorela added a comment.


  In D14529#302624 , @aacid wrote:
  
  > Ok, second option, we are actually installing these files ourselves, right?
  
  
  It is where androiddeployqt puts things (I guess to avoid conflict with 
javastuff)

REPOSITORY
  R240 Extra CMake Modules

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

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

D14896: Fix bzip main page

[Sune Vuorela]

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


  yesplease.

REPOSITORY
  R243 KArchive

BRANCH
  master

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

To: ltoscano, svuorela
Cc: svuorela, kde-frameworks-devel, michaelh, ngraham, bruns

D14772: Deprecate KFilterProxySearchLine

[Sune Vuorela]

This revision was automatically updated to reflect the committed changes.
Closed by commit R276:f5fef035e5a2: Deprecate KFilterProxySearchLine (authored 
by svuorela).

REPOSITORY
  R276 KItemViews

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14772?vs=39579=39580

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

AFFECTED FILES
  src/kfilterproxysearchline.h

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

D14772: Deprecate KFilterProxySearchLine

[Sune Vuorela]

svuorela updated this revision to Diff 39579.
svuorela added a comment.


  Improve deprecation documentation

REPOSITORY
  R276 KItemViews

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14772?vs=39528=39579

BRANCH
  master

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

AFFECTED FILES
  src/kfilterproxysearchline.h

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

D14772: Deprecate KFilterProxySearchLine

[Sune Vuorela]

svuorela created this revision.
svuorela added reviewers: kossebau, dfaure.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
svuorela requested review of this revision.

REVISION SUMMARY
  It is too primitive and simple for anything but the absolutely most basic 
things
  
  It is fixed to FixedStrings, search in all columns and caseinsensitive. And 
it seems to
  not really be fixeable. So just  mark it as deprecated and remove it at a 
later point.

TEST PLAN
  It has approximately 6 users across the kde codebase, let's fix them one by 
one.

REPOSITORY
  R276 KItemViews

BRANCH
  master

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

AFFECTED FILES
  src/kfilterproxysearchline.h

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

D14565: Fix overflow in rounding code

[Sune Vuorela]

This revision was automatically updated to reflect the committed changes.
Closed by commit R244:ea2779611800: Fix overflow in rounding code (authored by 
svuorela).

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14565?vs=39024=39025

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

AFFECTED FILES
  autotests/kformattest.cpp
  src/lib/util/kformatprivate.cpp

To: svuorela, cfeck, safaalfulaij
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14565: Fix overflow in rounding code

[Sune Vuorela]

svuorela added a comment.


  In D14565#302581 , @cfeck wrote:
  
  > In D14565#302551 , @safaalfulaij 
wrote:
  >
  > > Maybe you should use `largeValue` instead of `largevalue`.
  >
  >
  > Yes, that would be consistent with what we do elsewhere.
  
  
  Done.
  
  > I had hoped a junior would grab this ticket before a senior ruins the show 
(insert emoticon here), but having a fast fix for 5.49.0 is surely also nice.
  
  I was considering leaving it for another week or two, but then I would 
probably have forgotten about it.

REPOSITORY
  R244 KCoreAddons

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

To: svuorela, cfeck, safaalfulaij
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14565: Fix overflow in rounding code

[Sune Vuorela]

svuorela updated this revision to Diff 39024.
svuorela added a comment.


  s/largevalue/largeValue/

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14565?vs=38978=39024

BRANCH
  master

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

AFFECTED FILES
  autotests/kformattest.cpp
  src/lib/util/kformatprivate.cpp

To: svuorela, cfeck, safaalfulaij
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14565: Fix overflow in rounding code

[Sune Vuorela]

svuorela created this revision.
svuorela added reviewers: cfeck, safaalfulaij.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
svuorela requested review of this revision.

REVISION SUMMARY
  qRound works with int.
  
  BUG: 397008

TEST PLAN
  add unit tests

REPOSITORY
  R244 KCoreAddons

BRANCH
  master

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

AFFECTED FILES
  autotests/kformattest.cpp
  src/lib/util/kformatprivate.cpp

To: svuorela, cfeck, safaalfulaij
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14400: Fix Mixed to Upper mode latching in Aztec code generation

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R280 Prison

BRANCH
  master

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

To: vkrause, svuorela
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

Re: Installing qml caches

[Sune Vuorela]

On 2018-06-04, Alexander Volkov  wrote:
> This can be made optional, as it is done in Qt.
> So distros will have a choice whether to install qmlc files or not.
> Debian installs them.

I think Debian only installs the qmlc files for Qt packages (where the
versioning already is tightly coupled to Qt stuff)

/Sune

D12477: Add unit test to see that :/ files can work

[Sune Vuorela]

svuorela created this revision.
svuorela added reviewers: dfaure, kossebau, Frameworks.
Restricted Application added a project: Frameworks.
svuorela requested review of this revision.

REVISION SUMMARY
  I wasn't sure if :/foo stuff would work, so I tried writing a unit test to 
help me. Let's submit it. It passes

TEST PLAN
  Unit test still passes.

REPOSITORY
  R306 KParts

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/parttest.cpp
  autotests/parttest.h

To: svuorela, dfaure, kossebau, #frameworks
Cc: michaelh, bruns

D10826: Introduce DocumentId class

[Sune Vuorela]

svuorela added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in documentid.h:51-52
> Make a d poniter in exported class.

it depends on why it is exported. If it is exported just for unit test (and the 
header file not installed), then we don't need to have the mental and code wise 
overhead of a d-pointer.

If we know that it will never ever need to grow, we also don't need the 
overhead of a d-pointer. 
So it is a big "it depends", which I was also kind of trying to ask into 
earlier.

REPOSITORY
  R293 Baloo

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

To: michaelh, adridg, #baloo, #frameworks
Cc: anthonyfieroni, svuorela, ashaposhnikov, michaelh, spoorun, nicolasfella, 
alexeymin

D10826: Introduce DocumentId class

[Sune Vuorela]

svuorela added a comment.


  A quick review of some quirks and weirdnesses in c++

INLINE COMMENTS

> documentid.cpp:67
> +#if(0)
> +// Due to operator DeviceIdAndInode(), apparently the following
> +// became obsolete.

if you make operator DeviceIdAndInode explicit, you probably end up with a bit 
better behavior.

Else the compiler happily converts a documentid to a deviceidandinode in order 
to make various comparisons at compile time.

e.g. DocumentId id(5,5);
bool b = true;
if (id == b) { ... }  should compile, but probably isn't intended behavior..

> documentid.h:34
> +
> +class BALOO_ENGINE_EXPORT DocumentId
> +{

Does it need to be exported? Is things outside of the baloo engine need to 
access this? (or is it just avaliable for tests)

If it is public-public, you need to be careful about the class size and ensure 
you have enough bits to work with in the members.

> documentid.h:41
> +//TODO: DocumentId(const QT_STATBUF& stBuf);
> +~DocumentId();
> +

is this needed? or is the default generated good enough?

> documentid.h:45
> +
> +DocumentId operator=(const DeviceIdAndInode devino);
> +DocumentId operator=(const DocumentId& id);

shouldn't this return a DocumentId& rather than a copy?

> documentid.h:46
> +DocumentId operator=(const DeviceIdAndInode devino);
> +DocumentId operator=(const DocumentId& id);
> +

=default

  or if you remove the custom dtor, this will be autogenerated.

REPOSITORY
  R293 Baloo

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

To: michaelh, adridg, #baloo, #frameworks
Cc: svuorela, ashaposhnikov, michaelh, kmorwinski, spoorun, nicolasfella, 
alexeymin

D10433: Add QML support for Prison

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R280 Prison

BRANCH
  master

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

To: vkrause, #frameworks, svuorela
Cc: broulik, davidedmundson, michaelh

D10433: Add QML support for Prison

[Sune Vuorela]

svuorela added a comment.


  I am a bit unsure if this is the right approach. I can still be convinced 
both ways.
  
  one of the big differences from prison/qt4 to prison/qt5 was that it changed 
from being a barcode display library to a barcode generation library, removing 
all means of actually displaying them.
  
  I don't know if a better approach would be to still not actually display 
anything, but do a bit more to make it possible to just use a AbstractBarcode 
in a QImageItem.
  
  I am also not sure how a custom AbstractBarcode could be fitted in here, and 
if it is actually relevant to care for a thirdparty AbstractBarcode 
implementation.
  
  This is a bit of rambling, but I'm not fully sure what way to sway. The code 
as such is good, but I'd like to just bounce these questions around first.

REPOSITORY
  R280 Prison

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

To: vkrause, #frameworks, svuorela
Cc: broulik, davidedmundson, michaelh

D10631: Update links to dependencies, and mark Android as officially supported

[Sune Vuorela]

svuorela accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R280 Prison

BRANCH
  master

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

To: vkrause, svuorela
Cc: #frameworks, michaelh