Re: Is there (going to be) an auto-retracer service for KDE?

[Harald Sitter]

On Tue, Apr 27, 2021 at 2:16 PM Halla Rempt  wrote:
>
> On Monday, 26 April 2021 13:07:17 CEST Harald Sitter wrote:
> > Also going off on a tangent: On windows I understand the store already
> > has crash tracking and all that stuff implemented, I expect the same
> > is true for OSX?
>
> Yes, but without proper debug symbols the traces are not all that useful -- 
> though still good enough to tell me that display drivers on Windows are the 
> main source of crashes.

Our apps that I have access to all seem to have symbols uploaded. That
being said the "stack trace" isn't too hot in general:

> FrameImageFunctionOffset
> 0Qt5Core.dllqt_message_fatal0x0018
> 1Qt5Core.dllQMessageLogger::fatal0x0071
> 2Qt5Gui.dllinit_platform0x0861

Which, I mean, is a call trace, but that's all it is. Fortunately one
can actually get a hold of the minidump with a huge caveat:

https://docs.microsoft.com/en-us/windows/uwp/publish/health-report

> CAB files will only be available when the failure occurred on a computer 
> using a Windows Insider build, so not all failures will include the CAB 
> download option.

With that in mind I guess the answer is that crash reports from
systems that aren't in insider mode aren't terribly useful in general
outside the general sense that something may be crashing a whole lot
and which functions are involved.
Crash reports from insiders OTOH have this zip file available which
contains the WER [1] metadata and the minidump which one can load into
VS [2] or windbg [3]. Combined with auto-retrieval [4] that surely
should give all the building blocks to generate rich(er) traces.

This really doesn't seem quite as useless as you make it sound. It's
still a bit meh to need insider users to get viable data though. What
might be interesting there is if an app could opt into WER local
dumping and then have a dialog on next run "yo, krita crashed last
time, how about you send us the crash" which would then behave like
what I described for desktop linux (upload minidump to us, we trace
server-side) thereby bypassing microsoft entirely. Albeit this has all
the same challenges as desktop linux too then. Someone needed to write
the weby-techy bits of receiving dumps and then tracing them and
manage a crash record database.

[1] https://docs.microsoft.com/en-us/windows/win32/wer/windows-error-reporting
[2] https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files
[3] 
https://docs.microsoft.com/en-us/windows-hardware/drivers/debugger/debugger-download-tools
[4] 
https://docs.microsoft.com/en-us/windows/uwp/monetize/access-analytics-data-using-windows-store-services

HS

Re: Is there (going to be) an auto-retracer service for KDE?

[Harald Sitter]

On Mon, Apr 26, 2021 at 12:34 PM Halla Rempt  wrote:
>
> On Monday, 26 April 2021 11:26:38 CEST Harald Sitter wrote:
> > * get a core instead of a trace - sounds simpler than it is because
> > cores can be huge and network speed may be limited. google's breakpad
> > has a special minidump format that seeks to deal with that, I think
> > apport also has its own reduction format.
>
> I've twice in my life integrated breakpad into software, and it was hell both 
> times.

I did not mean to suggest  that we should use breakpad. In fact my
ephemeral plan, as far as desktop Linux goes, would be to rely on
systemd's coredumpd (drkonqi integration for that is actually what I'm
working on right now). Regardless of "how" the crash gets intercepted
we need a way to send cores over the wire and for that we may need a
better format. Plasma cores are upwards of 50MiB for me, even when
compressed.

> Generating and updating and managing the symbol files was a continuous chore, 
> too.

Luckily distros have that sorted out already ;)

Also going off on a tangent: On windows I understand the store already
has crash tracking and all that stuff implemented, I expect the same
is true for OSX? No idea about android, flatpak and snap though. In
general I would argue that we should rely on the platform crash
tracking systems more. On desktop linux I'd let cores fall into
coredumpd and then fish them out of there. On freebsd and linuxes
without systemd we can simply do what we always did - to drkonqi it
makes no diff if it traces the process or generates a core. Of course
in either case we'd need a way to send and then trace crashes
server-side on our end.

HS

Re: Is there (going to be) an auto-retracer service for KDE?

[Harald Sitter]

On Sun, Apr 25, 2021 at 10:38 PM Albert Astals Cid  wrote:
> KDE can't do retracing because KDE hasn't built the packages people use 
> (except KDE Neon).

I've mused on this in the past and I think it can be done, it's just a
lot of work that in particular also will need to involve some web
software to upload to, and manage crashes in.

In broad strokes what needs to happen is:

* get a core instead of a trace - sounds simpler than it is because
cores can be huge and network speed may be limited. google's breakpad
has a special minidump format that seeks to deal with that, I think
apport also has its own reduction format.
* get all the metadata we can find - this likely needs distro
integration depending on the data. this will need (e.g.) a list of
either all installed binary packages and their version, or at least
the ones supplying the files loaded in memory
* upload all the data to a web service and put it in a processing queue
* send all the data off to a distro-specific retracing service - the
distros would need to supply/maintain this. it'd setup an env as
similar to the crashing system as possible, then run a series of gdb
commands to then send the output back to us (this could conceivably be
as simple as a dockerfile)
* we parse the output (which would include a trace) to then either
record a new crash or attach the data to an existing crash

All of this ideally without falling for alluring NIH solutions.

It'd be a huge undertaking and is a bit undermined by the rather
excellent debuginfod [1] that makes our existing way of doing things
more accessible, albeit network-wise just as expensive.

[1] https://sourceware.org/elfutils/Debuginfod.html

For further reference:

- https://abrt.readthedocs.io/en/latest/
- https://launchpad.net/errors

HS

Re: Including instead of , does it upset POSIX?

[Harald Sitter]

To conclude this our verdict is to always use the modern headers?

On Wed, Apr 14, 2021 at 3:13 PM Ahmad Samir  wrote:
>
> Hello :)
>
> A week or so ago I created an MR to include  instead of  in 
> KIO[1].
>
>  From /usr/include/c++/10/cerrno:
> /** @file cerrno
>   *  This is a Standard C++ Library file.  You should @c \#include this file
>   *  in your programs, rather than any of the @a *.h implementation files.
>   *
>   *  This is the C++ version of the Standard C Library header @c errno.h,
>   *  and its contents are (mostly) the same as that header, but are all
>   *  contained in the namespace @c std (except for names which are defined
>   *  as macros in C).
>   */
>
>
> And then I made similar commits to a lot of the other Frameworks (not all, 
> since the build failed
> for some of them, so I left them alone).
>
> Harald Sitter asked me to raise the point here, basically he's wondering 
> whether this change might
> cause issues, not being 100% POSIX compliant...etc; I'll quote him because he 
> explained it much
> better than I would:
>
>  ahmadsamir:
> https://invent.kde.org/frameworks/kcrash/-/commit/7a20755723dc1527edb7ed5c3fdcccdbcf7fa768
>  was this
> ever discussed anywhere? cause there are others like this and my thinking up 
> until now was that
> using the C .h is more appropriate since we use them for POSIX APIs and from 
> that POV the POSIX
> specified header is errno.h
>
> [...]
>
>  might be worth raising for discussion on the mailing list. with 
> c++>=14 there's a whole
> mountain of "deprecated" headers OTOH I also recall reading that the c++ 
> standard doesn't guarantee
> compatibility
>  which may or may not be a concern when we use stuff specifically 
> with the expectation that
> they will behave as documented from a POSIX or linux POV (such as signal 
> safety in kcrash)
>
>
> [1] https://invent.kde.org/frameworks/kio/-/merge_requests/397
> --
> Ahmad Samir

Re: Including instead of , does it upset POSIX?

[Harald Sitter]

FWIW this goes beyond errno. See clang-tidy's
modernize-deprecated-headers for a comprehensive list

HS

Re: Including LayerShellQt in Plasma in time for 5.22

[Harald Sitter]

- I'm like a broken record at this point: some of the cmake files and
the json file lack licensing details. please run `reuse lint` to check
the list. it's incredibly close to being fully licensed
- you include KDEClangFormat but don't actually seem to enable it
- along a similar note: the code style is inconsistent at times, you
might want to also enable the githooks when built with a new enough
ECM
- CheckIncludeFiles is also included but not used
- you sometimes use the Qt5::Foo targets rather than the Qt::Foo
targets, I believe the latter is preferred for easier porting to qt6
- the library code calls qputenv and ignores its return value, might
make sense to qcwarning when the env variable failed to set

other than that it looks lovely 

HS

Re: KQuickImageEditor in KDEReview

[Harald Sitter]

- really close to 100% reuse cover. plz consider adding data to
KQuickImageEditorConfig.cmake.in and src/controls/plugins.qmltypes
- in fact... shouldn't the qmltypes file be generated at build time?
- there's commented out KDEFrameworkCompilerSettings in CMakeLists,
please remove it, Friedrich asked us to not use it outside frameworks
and having it commented out only tempts people into using it ;)
- as with koko, it might make sense to bump the kf5 requirement to
5.79 and use clang format+hooks. there are some stilistic
inconsistency within the code already
- you should consider adding an option() definition for the
BUILD_SHARED_LIBS variable so it is documented and expicitly
initialized to a default value, I guess
- ResizeRectangle has raw pointers that aren't initialized to nullptr by default
- resizerectangle.cpp includes moc_resizerectangle.cpp is there a
reason for that? shouldn't automoc magic do this on its own?
- same for resizehandle.cpp

some typos in imagedocument.h:
  Mirrror
  horizonal (actually in mirrocommand.{h,cpp} as well)
  operattion

HS

Re: Koko in KDEReview

[Harald Sitter]

On 16.03.21 20:10, Carl Schwan wrote:
> Le mardi, mars 16, 2021 12:55 PM, Harald Sitter  a écrit :
> 
>> On Tue, Mar 16, 2021 at 12:49 PM Harald Sitter sit...@kde.org wrote:
>>
> 
>>> Yay!
> 
> Thanks for the big review :)
> 
>>>
> 
>>> -   please get a bugzilla produce created for it
> 
> Not a fan of that. This will ends up exactly like the www bugs, something
> that I look into every 6 months. We already have many issues opened in
> invent and it's working fine for us. 

Did I miss something? The last agreement I recall was that we are using
bugzilla for bugs and gitlab for tasks for the time being. If even as
developer I have to go hunting where $project tracks their bugs I'm sure
not going to be a happy camper.

> Also we don't use KCrash.

Shouldn't you?

>>> While koko is running, if I copy a file with geodata to the pictures
>>> dir it crashes on `kd_insert3 (tree=0x0,` supposedly because the
>>> geocoder had been deinit()'d while it was still running. most notably
>>> Processor (which is the gui thread) calls deinit on the geocoder
>>> (which is used from various runner threads). in point of fact,
>>> glancing over the code I'm not convinced this is actually correctly
>>> threading
>>> ImageProcessorRunnable is a runnable that is put into the thread pool.
>>> Inside its run there's
>>>
> 
>>> if (!m_geoCoder->initialized()) {
>>> m_geoCoder->init();
>>> }
>>> 
> 
>>>
> 
>>> This is racing code. In between the call to initialized() and the
>>> init() another thread might have done init() already. At best that is
>>> leaking kd_create instances, at worst that crashes on one of the many
>>> asserts on members. On top of that Processor then also calls into
>>> deinit() from its thread which might be at any point in time while the
>>> Runnable's run() runs. The entire construct is lacking any sort of
>>> appreciation for thread synchronization. This needs at least a mutex
>>> to synchronize the init/deinit and possibly lookup if kd_* is not
>>> thread safe.
>>> I am not sure if the init-deinit dance is really adding much value
>>> here. If it isn't I might just do away with the deinit TBH.
>>> HS
> 
> This code is now using a ReadWriteLock. This should fix the racing code. 

Still crashes when I move an image with geodata into the picture dir :(

at ../src/reversegeocoder.cpp:151

>> Oh! Three follow ups:
>>
> 
>> -   is it intentional that this isn't a uniqueapp [1]? do multiple
>> concurrent koko instances even work with the database?
> 
> It is intentional since users can open image from Dolphin while Koko is
> already running. Maybe I could look into using KDSingleApplication for
> this usecase. Also multiple koko instances seems to be working in my
> experience but I didn't really test that in details.

That's what [1] does.
You register on kdbusservice register as org.kde.koko and connect to
activateRequested so when the user clicks on an image while koko is
already running that opening request is sent to the running instance
instead of starting a completely new one.

>> -   the geodata magic doesn't seem to be working for me. it correctly
>> maps the geodata in ReverseGeoCoder but in the UI nothing is displayed
>> under locations
> 
> Could you take a look into `~/.local/share/koko/imageData.sqlite3` with
> sqlitebrowser and tell me if the locations and image are correctly tagged?
> Also try to remove `~/.local/share/koko/imageData.sqlite3` and 
> `~/.local/share/koko/fstracker.sqlite3`
> after pulling the project new, this might have been caused by the racing
> code from above.

Works after I wiped config and data to start with a fresh set of data
:shrug:

HS

Re: Koko in KDEReview

[Harald Sitter]

On Tue, Mar 16, 2021 at 12:49 PM Harald Sitter  wrote:
>
> Yay!
>
> - please get a bugzilla produce created for it
> - COPYING-CMAKE-SCRIPTS seems unused
> - the find_package calls on qt5 probably should be versioned on
> whatever version you actually tested with, currently it's unversioned
> which I doubt is correct
> - same for kquickimageeditor
> - kquickimageeditor is found with an empty COMPONENTS list. is that 
> intentional?
> - unless you specifically target kf5.70 it might make sense to bump to
> .79 and use KDEGitCommitHooks so clang-format is more consistently
> applied
> - kdtree.{c,h} references BSD-3-Clause but that license isn't in the source
> - on the topic of licensing: since the code base is really close to
> complete reuse coverage it might be nice to push it over the finishing
> line and then `reuse lint` it to not have it fall behind again
> - some classes have trivial constructors/destructors and could use
> =default (e.g. roles.cpp, types.cpp, processor.cpp, openfilemodel.cpp,
> probably others)
> - some of those are further QObjects that have a parent signature but
> don't delegate construction correctly (i.e. the parent arg is never
> forwarded to QObject). since they are also trivial the constructors
> could be removed and replaced by `using QObject::QObject;` to adopt
> QObject's ctors (e.g. roles.cpp, types.cpp)
> - some of the .h's have `parent = 0`, it'd be nice to have them use
> nullptr instead
> - some functions take references when they should take const refs
> (e.g. `void setPath(QString )` or the ImageProcessorRunnable ctor)
> this suggests they mutate the object, but really the mentioned
> functions don't
> - for future reference: .appdata.xml is a legacy suffix and
> .metainfo.xml ought to be used instead. alas, no point changing this
> now to avoid extra work for the i18n team
> - the appstream file must use the CDN not a wiki for screenshots
> https://invent.kde.org/websites/product-screenshots
> - also generally speaking please don't generate or use thumbs for
> appstream files, appstream-generator will thumbnail the raw images
> during assembling stage on the distro side of things
> - appstream data has a help link that goes nowhere
>
> While koko is running, if I copy a file with geodata to the pictures
> dir it crashes on `kd_insert3 (tree=0x0,` supposedly because the
> geocoder had been deinit()'d while it was still running. most notably
> Processor (which is the gui thread) calls deinit on the geocoder
> (which is used from various runner threads). in point of fact,
> glancing over the code I'm not convinced this is actually correctly
> threading
>
> ImageProcessorRunnable is a runnable that is put into the thread pool.
> Inside its run there's
>
> if (!m_geoCoder->initialized()) {
> m_geoCoder->init();
> }
>
>
> This is racing code. In between the call to initialized() and the
> init() another thread might have done init() already. At best that is
> leaking kd_create instances, at worst that crashes on one of the many
> asserts on members. On top of that Processor then also calls into
> deinit() from its thread which might be at any point in time while the
> Runnable's run() runs. The entire construct is lacking any sort of
> appreciation for thread synchronization. This needs at least a mutex
> to synchronize the init/deinit and possibly lookup if kd_* is not
> thread safe.
>
> I am not sure if the init-deinit dance is really adding much value
> here. If it isn't I might just do away with the deinit TBH.
>
> HS

Oh! Three follow ups:

- is it intentional that this isn't a uniqueapp [1]? do multiple
concurrent koko instances even work with the database?
- the geodata magic doesn't seem to be working for me. it correctly
maps the geodata in ReverseGeoCoder but in the UI nothing is displayed
under locations
- since it doesn't appear in the UI, I can't check: is the geodata
actually getting localized? at least the geocoder seems to spit out
non-localized values

[1] https://api.kde.org/frameworks/kdbusaddons/html/classKDBusService.html

Re: Koko in KDEReview

[Harald Sitter]

Yay!

- please get a bugzilla produce created for it
- COPYING-CMAKE-SCRIPTS seems unused
- the find_package calls on qt5 probably should be versioned on
whatever version you actually tested with, currently it's unversioned
which I doubt is correct
- same for kquickimageeditor
- kquickimageeditor is found with an empty COMPONENTS list. is that intentional?
- unless you specifically target kf5.70 it might make sense to bump to
.79 and use KDEGitCommitHooks so clang-format is more consistently
applied
- kdtree.{c,h} references BSD-3-Clause but that license isn't in the source
- on the topic of licensing: since the code base is really close to
complete reuse coverage it might be nice to push it over the finishing
line and then `reuse lint` it to not have it fall behind again
- some classes have trivial constructors/destructors and could use
=default (e.g. roles.cpp, types.cpp, processor.cpp, openfilemodel.cpp,
probably others)
- some of those are further QObjects that have a parent signature but
don't delegate construction correctly (i.e. the parent arg is never
forwarded to QObject). since they are also trivial the constructors
could be removed and replaced by `using QObject::QObject;` to adopt
QObject's ctors (e.g. roles.cpp, types.cpp)
- some of the .h's have `parent = 0`, it'd be nice to have them use
nullptr instead
- some functions take references when they should take const refs
(e.g. `void setPath(QString )` or the ImageProcessorRunnable ctor)
this suggests they mutate the object, but really the mentioned
functions don't
- for future reference: .appdata.xml is a legacy suffix and
.metainfo.xml ought to be used instead. alas, no point changing this
now to avoid extra work for the i18n team
- the appstream file must use the CDN not a wiki for screenshots
https://invent.kde.org/websites/product-screenshots
- also generally speaking please don't generate or use thumbs for
appstream files, appstream-generator will thumbnail the raw images
during assembling stage on the distro side of things
- appstream data has a help link that goes nowhere

While koko is running, if I copy a file with geodata to the pictures
dir it crashes on `kd_insert3 (tree=0x0,` supposedly because the
geocoder had been deinit()'d while it was still running. most notably
Processor (which is the gui thread) calls deinit on the geocoder
(which is used from various runner threads). in point of fact,
glancing over the code I'm not convinced this is actually correctly
threading

ImageProcessorRunnable is a runnable that is put into the thread pool.
Inside its run there's

if (!m_geoCoder->initialized()) {
m_geoCoder->init();
}


This is racing code. In between the call to initialized() and the
init() another thread might have done init() already. At best that is
leaking kd_create instances, at worst that crashes on one of the many
asserts on members. On top of that Processor then also calls into
deinit() from its thread which might be at any point in time while the
Runnable's run() runs. The entire construct is lacking any sort of
appreciation for thread synchronization. This needs at least a mutex
to synchronize the init/deinit and possibly lookup if kd_* is not
thread safe.

I am not sure if the init-deinit dance is really adding much value
here. If it isn't I might just do away with the deinit TBH.

HS

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

[Harald Sitter]

On Tue, Jan 26, 2021 at 10:35 AM Halla Rempt  wrote:
>
> It turns out that we have several problems with loading translations in 
> Krita. Qt and ki18n translations seem to follow different rules with regards 
> to users setting a language other than the platform language, on different 
> platforms.
>
> But it also seems that the qm-based translations from the tier1 frameworks we 
> ship aren't loaded at all, unless the user is using distro packages for 
> everything, in a plasma environment. I can see code in ki18n that tries to 
> load the translations for Qt, but the frameworks don't seem to have that 
> startup hook, and I don't know how it's supposed to work...

The frameworks that are using Qt translation tech instead of gettext
will generally set up a loader somewhere in the code.

https://api.kde.org/ecm/module/ECMPoQmTools.html

"It assumes that the .qm file for the language code  is
installed as /locale//LC_MESSAGES/.qm,
where  is one of the directories given by the
GenericDataLocation of QStandardPaths."

HS

Re: New repo in kdereview: plasma-settings

[Harald Sitter]

On 25.01.21 08:05, Bhushan Shah wrote:
> Hello everyone!
> 
> I want to move plasma-settings repo to kdereview, it is settings
> application used by the Plasma Mobile, it is based on Kirigami.
> 
> https://invent.kde.org/plasma-mobile/plasma-settings
> 
> Main difference between plasma-settings and original system settings
> application used in plasma desktop is, this code is much more lean and
> modern, it is my understanding that long term idea is to replace the
> original systemsettings code base with plasma-settings. But for now this
> is mobile specific.

kpackage is very grumpy during cmake stage

no appstream data :(

Not sure if intentional but ModulesModel actually doesn't constrain to
settings KCMS. It'd also display info center kcms as well as (I think)
plugin KCMS (e.g. krunner's)

not installing a hicolor icon

I am pretty sure l10n isn't working. I can't see the translations domain
set anywhere (mind you, kcms likely work; --help might now)

Along a similar note the package/ dir isn't covered by a Messages.sh and
consequently isn't actually i18n'd. I suggest moving package into src/
and the main Message.sh into src/ as well and let it cover everything
inside. It'd clarify extraction responsibility somewhat.

might be worth making the source reuse compliant

some qml file lack license information (KCMContainer.qml, info's
main.qml at least - would be easier to check if the source was compliant ;))

I am very certain that lots of code inside modules/info/ was copied from
kinfocenter's about-distro but lacks any attribution to its original
authors *cough* including your's truly

password module has copies of org.freesktop.Accounts*xml at a glance
they are installed by accountsservice, you could just use the installed
versions instead of holding a copy

HS



signature.asc
Description: OpenPGP digital signature

Re: New repo in kdereview: plasma-angelfish

[Harald Sitter]

On 25.01.21 06:55, Bhushan Shah wrote:
> Hello everyone!
> 
> Yet another Plasma Mobile repository in kdereview: 
> 
> https://invent.kde.org/plasma-mobile/plasma-angelfish
> 
> Plasma Angelfish is browser written in Kirigami which uses the
> QtWebengine for rendering web pages. It is optimized for the mobile
> usecase.
> 
> Current feature list is available in README.md of the repository.

Not sure I am liking the InitialPreference in the desktop file.

Encoding in desktop files has been deprecated for like a million years ^^

Inconsistent spelling between desktop file "Web Browser" and appstream
file "Webbrowser". I also think the former is the way one generally
spells it.

.appdata.xml is a legacy suffix, you might want to use .metainfo.xml

Same as with phonebook, I don't see the translation domain getting set
anywhere and by extension l10n is likely kaput.

Inside SettingsNavigationBarPage.qml there's a concatenated multi line
string, I'm not sure if that'd get extracted correctly for l10n. Best check.

Many files don't have license info, see point 15 of the license policy
[1] :(

You may want to move from the incomplete COPYING file to reuse LICENSES/
folder.

DesktopFileGenerator manually calls kbuildsycoca :O - This should just
work and if not get fixed properly. Super alternatively
KSycoca::self()->ensureCacheValid() is exactly made for this use case.

The flatpak run inside DesktopFileGenerator seems super opinionated, but
with the only target system being plasma-mobile I guess it doesn't matter?

The message("warning:" in src/CMakeLists.txt ought to be
add_feature_info() really [2]

[1] https://community.kde.org/Policies/Licensing_Policy
[2] https://cmake.org/cmake/help/v3.8/module/FeatureSummary.html

> Before moving it to extragear I would like to drop plasma- prefix from
> the repository as this repo have nothing much to do with "Plasma". And
> it is otherwise already called just "Angelfish" in its desktop files and
> stuff.
+1




signature.asc
Description: OpenPGP digital signature

Re: New repo in kdereview: plasma-phonebook

[Harald Sitter]

On 25.01.21 06:48, Bhushan Shah wrote:
> Hello everyone!
> 
> I am back with more Plasma Mobile related repositories in kdereview. I
> want to move plasma-phonebook in kdereview. plasma-phonebook is kirigami
> based phonebook application, it uses kpeople backends like kpeoplesink,
> kpeoplevcard, kpeople akonadi backend etc to fetch and store contacts on
> your system.

I am pretty sure l10n isn't working. I can't see the translations domain
set anywhere.

On the subject of strings I'd suggest checking them all for HIG
compliance. In main.qml alone all strings are wrong as titles and
buttons are supposed to use Title Capitalization
https://hig.kde.org/style/writing/capitalization.html

The desktop file seems to be lacking an actual main category (it's in
the lost section in the launcher for me)

What's the use case for the unregistered x-plasma-phonebook mimetype
(registered for by the desktop file)? Perhaps the more relevant question
is why would it should be unregistered instead of inside our vendor tree
(vnd.kde.phone.book or some such)?

Many files don't have license info, see point 15 of the license policy
[1] :(

Actual qml **source** files have no license info!

DetailListItem.qml isn't used.

Are we quite sure that hardcoding colors is the way to go? :O

Header.qml actually has a crash for me because ColorOverlay is inside a
FastBlur that is both parent and source (qt docs about source: "Note: It
is not supported to let the effect include itself, for instance by
setting source to the effect's parent.")

Similarly the FastBlur looks to be falling into that trap of
parent===source, albeit not crashing for me.

Spelling is inconsistent. The desktop file spells it "Phone Book" the
appstream file spells it "Plasma Phonebook".

.appdata.xml is a legacy suffix, you might want to use .metainfo.xml
instead.

On the subject of the appstream file. Missing
KDE :(

# Probably a bit nitpicky

Failing to store contact changes in AddContactPage lacks actual UI
backing, it merely qwarns.

Various single argument ctors aren't tagged `explicit`.

[1] https://community.kde.org/Policies/Licensing_Policy

> Another thing I want to discuss is, branding, currently it is named as
> Plasma Phonebook which is bit awkward because Plasma is not related to
> app at all and even android port for app exists. Maybe we should name
> this just phonebook?

Is this about the repo/tarball name or the display string?

The former already lacks the plasma prefix in the .desktop file, I'd
argue the appstream name should do the same. System Settings is a
precedent for something similar.

As for tarballs: some distros get grumpy over overly generic source
names, so while I think phonebook is fine it may be less fine for
distros. It probably matters little what the repo/tarball is called though.

HS



signature.asc
Description: OpenPGP digital signature

Re: PSA: Please do not use KDEFrameworkCompilerSettings for non-KF projects

[Harald Sitter]

On 19.01.21 16:57, Friedrich W. H. Kossebau wrote:
> Hi,
> 
> the docs of the module tell us:
> "KDEFrameworkCompilerSettings - Set stricter compile and link flags for KDE 
> Frameworks modules."
> https://api.kde.org/ecm/kde-module/KDEFrameworkCompilerSettings.html
> 
> And it has been meant that way. E.g. because extending those settings with 
> more strict settings or new features would be done with other KF-specific 
> requirements or policies in mind, always matching those of the current 
> version 
> (given KF & ECM are released in-sync).
> Having to take care of backward-compatibility with non-KF usage and to do 
> proper documentation adds additional burden not wanted here.
> 
> As convenient it is to not have to write out e.g. all the 
> add_definitions(
> -DQT_NO_CAST_TO_ASCII
> -DQT_NO_CAST_FROM_ASCII
> -DQT_NO_URL_CAST_FROM_STRING
> -DQT_NO_CAST_FROM_BYTEARRAY
> -DQT_NO_SIGNALS_SLOTS_KEYWORDS
> -DQT_USE_QSTRINGBUILDER
> -DQT_NO_NARROWING_CONVERSIONS_IN_CONNECT
> )
> that needs some other solution.
> 
> For ECM/KF5 times the train has left, we need to handle those existing 
> abuses. 
> But please fix your projects now already by changing back to 
> KDECompilerSettings and the manual boilerplate, so in ECM/KF6 times that 
> burden is gone.

Perhaps there should be a unit test asserting this somehow?
LXR reports `247 occurences found.` so this seems a fairly wide spread
issue :(

Perhaps hardcode a list of all cmake project names that are frameworks
in ECM and then test that the settings are only used with one of them?
Just an idea.

HS

Re: RFC: suffix ".in" instead of ".cmake" for template files to substitution processing

[Harald Sitter]

On 04.01.21 08:21, Friedrich W. H. Kossebau wrote:
> Hi,
> 
> [replies only to kde-devel@kde.org, please]
> 
> tl;dr  This is a proposal to standardize on the suffix ".in" for input/
> template files passed to cmake's configure_file() in KDE projects. 
> 
> 
> When it comes to naming the files used as templates for build-time generation 
> of source/data files by substitution processing, by calling cmake's 
> configure_file() or configure_package_config_file(), there are currently two 
> traditions/cultures alive in KDE:
> * using the suffix ".in"
> * using the suffix ".cmake"
> Why is it like that I have some theories, but just stating the current 
> reality 
> here.
> 
> While having not too strict policies about how active developers manage their 
> sources is usually a better thing IMHO, I personally favour per-product 
> consistency only, in the case of input/template files though it can affect 
> things outside of a single project/repository, like with files which are 
> processed by the global KDE translation system:
> 
> As we discovered the last days, "scripty" (the nightly KDE translation system 
> bot that synchronizes between source repositories and the database repo of 
> the 
> translators) was only instructed to handle ".desktop.cmake" files, and thus 
> ignored the ".desktop.in" ones.
> While scripty last night now has been instructed to also start to care for 
> the 
> ".in" variant, having two different options adds some complexity and allows 
> some confusion for people looking at such files ("why once .cmake and once 
> .in? Is there a difference?"). And besides personal preference so far no real 
> argument for being able to choose a suffix was told.
> 
> So given that and first comments on the matter mostly in similar directions, 
> I 
> propose to have some global KDE standard on the suffix of such input/template 
> files, and once decided, also convert existing files to that suffix, so 
> people 
> do not revive the deprecated suffix culture due to innocent copy & paste :)
> 
> And the proposal is to use ".in".
> 
> Pros:
> * ".in" can be seen used as suffix with examples in documentation of at least 
> autotools, cmake, qmake & scons for input files to substitution processing, 
> and as result can be also found in many projects using those buildsystems. So 
> people switching between projects or new to KDE do not have to relearn or 
> switch their mindset
> * ".in" is shorter :)
> * ".cmake" is the suffix to denote CMake code files usually, including the 
> CMake config files (which are partially also generated from template files, 
> so 
> such would be named FooConfig.cmake.cmake, which at least looks strange), 
> overloading the meaning of the suffix makes things a bit less clear
> * [please fill in]
> 
> Cons:
> * some substitution markup is cmake-specific, like #cmakedefine, so projects 
> trying to support multiple build systems would like to specify the cmake-
> variant filetype some more (not aware of that need in KDE projects currently)
> * [please fill in]
> 
> 
> Your comments, please.
Seems like useless policy making. There is no benefit to being
consistent and considering cmake upstream doesn't enforce a standard I
don't see why we should. If we end up changing this we might as well
deal with .hpp and .hxx and .cxx and what have you.

Changing this stuff generates noise in the repos I for one can do without.

HS

Re: Merge tags in master branch?

[Harald Sitter]

Hej

On 23.11.20 16:11, Bhushan Shah wrote:
> Hello,
> 
> So I have one question regarding the how we do the framework versioning.
> Namely the tags,
> 
> So currently some packages have a versioned tags on their master branch,
> 
> i.e
> 
> karchive:
> 
> ➜ git describe
> v5.76.0-1-g7304c28
> 
> While in case of some frameworks where translations needs to be taken
> from svn, it is something super weird like,
> 
> kdesu:
> 
> ➜ git describe
> v5.2.0-234-gb7ba89f
> 
> Some packagers who package -git versions in their unstable repos check
> the git describe to figure out what is current revision of the package
> and having "wrong" version there bugs out weirdly.
> 
> Do anyone have any opinion on "merging" latest git tag in master branch?
> and potentially doing that for next releases as well?

At that point we could just have scripty push the translations on the
daily runs surely? There's little point taking a round trip through the
release scripting if we effectively end up having translations in the
repo anyway, so we could just treat them like the desktop/xml
translations and have scripty forward them from svn into git.

I'm rather +1 to streamlining the translation handling. At the same time
I'm not sure if only doing this only for frameworks is all that cool,
but then I suppose frameworks is the less complicated. *shrug* I guess ;)

HS



signature.asc
Description: OpenPGP digital signature

Re: CMake kauth_install_* with system kauth and user CMAKE_INSTALL_PREFIX

[Harald Sitter]

On 19.10.20 17:44, Ingo Klöcker wrote:
> On Montag, 19. Oktober 2020 17:22:22 CEST Milian Wolff wrote:
>> On Montag, 19. Oktober 2020 17:21:28 CEST Harald Sitter wrote:
>>> I'm not sure it's a good idea, but we could treat this like
>>> KDE_INSTALL_USE_QT_SYS_PATHS. By default we'll simply install to
>>> $CMAKE_PREFIX/share/polkit-1/actions/ unless explicitly configured with
>>> KDE_INSTALL_USE_POLKIT_PATHS in which case we'll use whatever polkit uses.
>>> What sets this apart from qt_syspath of course is that not using the
>>> polkit path basically never makes sense as it'd be broken. So maybe the
>>> default state should be ON and when one is sure that polkit isn't needed
>>> one could turn it OFF.
>>
>> Installing something that's never going to be used - why? I would much
>> rather prefer making it explicit by saying:
>>
>> Either you install it to the correct path, or you don't install it at all.

Fair point

> FWIW, for a very similar problem (udev rules instead of polkit rules) 
> bluez-qt 
> has the CMAKE flag INSTALL_UDEV_RULE to disable installation of udev rules.
> 
> If have this in my .kdesrc-buildrc:
> =
> options bluez-qt
> cmake-options -DINSTALL_UDEV_RULE=0
> end options
> =
> 
> So, maybe add a KDE_INSTALL_POLKIT_RULE[S] (or ..._ACTION[S]) flag to allow 
> disabling the installation of polkit stuff.

+1

HS



signature.asc
Description: OpenPGP digital signature

Re: CMake kauth_install_* with system kauth and user CMAKE_INSTALL_PREFIX

[Harald Sitter]

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.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")
> ```

Ohh! This is indeed plain silly. I had thought it would use the value
from pkgconfig :S

> 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.

I'm not sure it's a good idea, but we could treat this like
KDE_INSTALL_USE_QT_SYS_PATHS. By default we'll simply install to
$CMAKE_PREFIX/share/polkit-1/actions/ unless explicitly configured with
KDE_INSTALL_USE_POLKIT_PATHS in which case we'll use whatever polkit uses.
What sets this apart from qt_syspath of course is that not using the
polkit path basically never makes sense as it'd be broken. So maybe the
default state should be ON and when one is sure that polkit isn't needed
one could turn it OFF.

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

polkit-gobject-1's pkgconfig encodes the path. So that's easy.

HS



signature.asc
Description: OpenPGP digital signature

Re: CMake kauth_install_* with system kauth and user CMAKE_INSTALL_PREFIX

[Harald Sitter]

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/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?

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.

HS



signature.asc
Description: OpenPGP digital signature

Re: CMake kauth_install_* with system kauth and user CMAKE_INSTALL_PREFIX

[Harald Sitter]

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

HS



signature.asc
Description: OpenPGP digital signature

Re: stale MR triaging - you can help!

[Harald Sitter]

On 16.10.20 10:00, Albert Astals Cid wrote:
> El dimarts, 13 d’octubre de 2020, a les 15:53:10 CEST, Harald Sitter va 
> escriure:
>> Moin
>>
>> As promised in a previous mail I've set up a system that helps us find
>> stale MRs to ensure patches don't fall by the wayside.
>>
>> Lists are filed as issues on invent. To help out you can hit the bell to
>> subscribe to `New issue` events at
>> https://invent.kde.org/sysadmin/gitlab-triaging
>>
>> First list of stale MRs here:
>> https://invent.kde.org/sysadmin/gitlab-triaging/-/issues
>>
>> To deal with a stale MR you'll want to check the box and either make
>> sure someone takes a look at the MR (e.g. by @mentioning someone who may
>> have experience) or doing the review yourself.
>>
>> Unlike previously described I've refined the behavior a bit:
>>
>> a) MRs are not linked but listed with verbatim url, it sucks a bit but
>> it prevents MRs from updating due to the back-reference an actual
>> mention would cause
>> b) stale MRs are now MRs that have not seen activity within 4 weeks (up
>> from 2 - was woefully too short a time frame)
>> c) MRs without subscribers after 2+ weeks are recorded separately
>> d) MRs without subscribers after 1+ week where the author is not a KDE
>> dev are also recorded (currently a bit bugged as it turns out)
>>
>> The motivation with c and d is to prevent MRs from going stale to begin
>> with, ideally anyway.
> 
> Can we get the issues in each of those 4 categories in different sections of 
> the issue you create?
> 
> It makes more sense for me to look at d) than at some MR whose author is 
> Aleix (for example), if it's stale, he knows how to do how to un-stale it, 
> while a newbie may not know how to.

Indeed. That's actually how it works, we've already dealt with the
important ones this week though. So all that's left is the mountain of
otherwise randomly stale MRs ;)

https://invent.kde.org/sysadmin/gitlab-triaging/-/issues?scope=all=%E2%9C%93=closed

HS

Re: stale MR triaging - you can help!

[Harald Sitter]

On 16.10.20 10:00, Albert Astals Cid wrote:
> El dimarts, 13 d’octubre de 2020, a les 15:53:10 CEST, Harald Sitter va 
> escriure:
>> Moin
>>
>> As promised in a previous mail I've set up a system that helps us find
>> stale MRs to ensure patches don't fall by the wayside.
>>
>> Lists are filed as issues on invent. To help out you can hit the bell to
>> subscribe to `New issue` events at
>> https://invent.kde.org/sysadmin/gitlab-triaging
>>
>> First list of stale MRs here:
>> https://invent.kde.org/sysadmin/gitlab-triaging/-/issues
>>
>> To deal with a stale MR you'll want to check the box and either make
>> sure someone takes a look at the MR (e.g. by @mentioning someone who may
>> have experience) or doing the review yourself.
>>
>> Unlike previously described I've refined the behavior a bit:
>>
>> a) MRs are not linked but listed with verbatim url, it sucks a bit but
>> it prevents MRs from updating due to the back-reference an actual
>> mention would cause
>> b) stale MRs are now MRs that have not seen activity within 4 weeks (up
>> from 2 - was woefully too short a time frame)
>> c) MRs without subscribers after 2+ weeks are recorded separately
>> d) MRs without subscribers after 1+ week where the author is not a KDE
>> dev are also recorded (currently a bit bugged as it turns out)
>>
>> The motivation with c and d is to prevent MRs from going stale to begin
>> with, ideally anyway.
> 
> Can we get the issues in each of those 4 categories in different sections of 
> the issue you create?
> 
> It makes more sense for me to look at d) than at some MR whose author is 
> Aleix (for example), if it's stale, he knows how to do how to un-stale it, 
> while a newbie may not know how to.

Indeed. That's actually how it works, we've already dealt with the
important ones this week though. So all that's left is the mountain of
otherwise randomly stale MRs ;)

https://invent.kde.org/sysadmin/gitlab-triaging/-/issues?scope=all=%E2%9C%93=closed

HS

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

[Harald Sitter]

On Wed, Oct 14, 2020 at 11:33 AM Milian Wolff  wrote:
> Does anyone know if there's an overview page similar to [1], but for Linux
> distributions?

https://repology.org/project/gcc/versions is the closest thing I know.
Mileage may vary as distributions tend to have multiple compiler
versions available even within the same releases and they aren't
necessarily named simply 'gcc' or 'clang' but may have fancy version
pre-/suffixes.

FWIW, plasma already chose to use C++17 in places.

HS

stale MR triaging - you can help!

[Harald Sitter]

Moin

As promised in a previous mail I've set up a system that helps us find
stale MRs to ensure patches don't fall by the wayside.

Lists are filed as issues on invent. To help out you can hit the bell to
subscribe to `New issue` events at
https://invent.kde.org/sysadmin/gitlab-triaging

First list of stale MRs here:
https://invent.kde.org/sysadmin/gitlab-triaging/-/issues

To deal with a stale MR you'll want to check the box and either make
sure someone takes a look at the MR (e.g. by @mentioning someone who may
have experience) or doing the review yourself.

Unlike previously described I've refined the behavior a bit:

a) MRs are not linked but listed with verbatim url, it sucks a bit but
it prevents MRs from updating due to the back-reference an actual
mention would cause
b) stale MRs are now MRs that have not seen activity within 4 weeks (up
from 2 - was woefully too short a time frame)
c) MRs without subscribers after 2+ weeks are recorded separately
d) MRs without subscribers after 1+ week where the author is not a KDE
dev are also recorded (currently a bit bugged as it turns out)

The motivation with c and d is to prevent MRs from going stale to begin
with, ideally anyway.

HS

stale MR triaging - you can help!

[Harald Sitter]

Moin

As promised in a previous mail I've set up a system that helps us find
stale MRs to ensure patches don't fall by the wayside.

Lists are filed as issues on invent. To help out you can hit the bell to
subscribe to `New issue` events at
https://invent.kde.org/sysadmin/gitlab-triaging

First list of stale MRs here:
https://invent.kde.org/sysadmin/gitlab-triaging/-/issues

To deal with a stale MR you'll want to check the box and either make
sure someone takes a look at the MR (e.g. by @mentioning someone who may
have experience) or doing the review yourself.

Unlike previously described I've refined the behavior a bit:

a) MRs are not linked but listed with verbatim url, it sucks a bit but
it prevents MRs from updating due to the back-reference an actual
mention would cause
b) stale MRs are now MRs that have not seen activity within 4 weeks (up
from 2 - was woefully too short a time frame)
c) MRs without subscribers after 2+ weeks are recorded separately
d) MRs without subscribers after 1+ week where the author is not a KDE
dev are also recorded (currently a bit bugged as it turns out)

The motivation with c and d is to prevent MRs from going stale to begin
with, ideally anyway.

HS

Re: KDiagram - Persistent FTBFS for stable branch on Windows

[Harald Sitter]

On 12.10.20 11:23, 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,
> 
> 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.

Just FTR for everyone's benefit:
https://community.kde.org/Guidelines_and_HOWTOs/Build_from_source/Windows#Virtual_Machines

Ever so useful to have if one deals with windows breakage more than once
a year ;)

HS



signature.asc
Description: OpenPGP digital signature

Re: KDiagram - Persistent FTBFS for stable branch on Windows

[Harald Sitter]

On 12.10.20 11:23, 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,
> 
> 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.

Just FTR for everyone's benefit:
https://community.kde.org/Guidelines_and_HOWTOs/Build_from_source/Windows#Virtual_Machines

Ever so useful to have if one deals with windows breakage more than once
a year ;)

HS



signature.asc
Description: OpenPGP digital signature

D28745: Skip caching thumbnails on encrypted filesystems

[Harald Sitter]

sitter added a comment.


  > Could this be delegated onto someone who knows Solid or should I try 
figuring it out by myself?
  
  Alas, I'm not sure there is anyone who feels responsible enough for solid, so 
you'll probably have to give it a go yourself.

REPOSITORY
  R320 KIO Extras

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

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure
Cc: sitter, dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, 
kfm-devel, waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, 
LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Harald Sitter]

sitter added a comment.


  In D28745#676452 , @marcingu wrote:
  
  > !PING.
  >  I need help from someone with good understanding of Solid to continue.
  >
  > I'm don't know how to determinate if StorageAccess device is encrypted or 
not. I wanted to use  `StorageVolume::usage`, but it's not available for all 
types of devices and doesn't equal `encrypted` for LUKS encrypted volumes.
  
  
  At a glance the problem here is actually that depending on the setup the 
mounted volume isn't necessarily the encrypted volume. e.g. if you make a LUKS 
encrypted btrfs on /dev/sda1 then sda1 is type LUKS encrypted, but to actually 
use it that device gets decrypted and mapped into a different name 
/dev/mapper/luks-123 which is type btrfs and not encrypted. It is only the 
mapped one that gets mounted which is why the encryption context gets lost 
along the way.
  
  I'm afraid this will need some adjustment in solid because currently we don't 
carry that information anywhere that I can see. It is however in the data of 
udiks2 behind the scenes (dbus property 
org.freedesktop.UDisks2.Block.CryptoBackingDevice which is a dbuspath of the 
encrypted backing object) so it should be readily available, just needs putting 
somewhere in solid. This is also represented in the sysfs through a slave 
relationship between the two block devices, but I'm guessing using that over 
udiks2 isn't nearly as portable.
  Where to put it I don't really know, probably a new method on one of the 
interface classes CryptoBackingUDI which returns the UDI representing the 
backing device.
  
  On an entirely unrelated note I for one would simplify the 
sharesFilesystemWithThumbRoot to check if the thumb root is encrypted and 
always cache the thumb if that is the case. Whether or not the devices are the 
same seems unnecessarily nitpicky so long as the results are encrypted if the 
originals were. Otherwise you run into awkward cases where a users has a system 
drive and a data drive, both encrypted, but because they are different we 
effectively disable all thumbnail caching for the data drive.

REPOSITORY
  R320 KIO Extras

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

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure
Cc: sitter, dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, 
kfm-devel, waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, 
LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

Re: plasma-systemmonitor in kdereview

[Harald Sitter]

On 01.10.20 14:36, Arjen Hiemstra wrote:
> On Thursday, 1 October 2020 14:11:16 CEST Harald Sitter wrote:
>> On 01.10.20 11:36, Arjen Hiemstra wrote:
>>> Hello,
>>>
>>> I'd hereby like to announce that plasma-systemmonitor is in kdereview. It
>>> can be found at https://invent.kde.org/plasma/plasma-systemmonitor .
>>>
>>> plasma-systemmonitor is a new system monitor UI built with Kirigami. It
>>> makes use of the ksystemstats daemon and the faces system for system
>>> monitor plasmoids that were both introduced in Plasma 5.19.
>>>
>>> Our current plan is to do a "preview release" alongside Plasma 5.20, then
>>> have it be an official part of Plasma with 5.21.
>>
>> Cool stuff.
>>
>> L10n is currently a bit incomplete.
>> Notably
>> - the pages files lack any localization at all and I'm also not sure how
>> those could be best localized.
> 
> Yeah that is a good point. If we can somehow extract the strings from these 
> files I think we can make it work. But that may need some custom scripting.
> 
>> - the 'configure columns' feature doesn't load the translated strings
>> into the checkboxes
> 
> Huh, weird. They are being populated by i18n calls, wonder what's going on 
> there.

I also couldn't spot what's going wrong FWIW. There was a problem with
strings getting loaded before the domain was set up or something with
qml plugins. Maybe it's that problem? OTOH why would that affect the
combobox but not the labels. It's very odd indeed.

>>
>> On the subject of configure columns I feel like word wrapping would look
>> better than eliding there https://i.imgur.com/RbAeUhH.png
>>
>> Something is astray with memory calculation. See konsole which is
>> somehow listed as 2.6gib https://i.imgur.com/MjIpj3O.png but its actual
>> children aren't getting nowhere near that number. I'm guessing it's
>> because even detached processes (notably vscode here) are in the same
>> cgroup? If so we probably can't do much about this?
>>
> 
> There's two things going on here I expect: everything started from konsole 
> will be part of the konsole cgroup unless started through kstart5 or a 
> similar 
> tool that explicitly moves things to a new cgroup. I doubt this is solvable 
> without patching all major shells. 
> 
> The other thing is that the "memory" column here is what is called "Total 
> Memory" in KSysGuard, which is the process' private memory with the shared 
> memory added divided over the processes that share that memory. This provides 
> a more accurate number for the actual memory usage of the process, but does 
> mean that it will most likely seem like it suddenly uses more memory, since 
> the "memory" in KSysGuard is only the private memory.

Makes sense.
Maybe we should blacklist konsole or something ^^
I'm almost certain people are going to look at it and go "gosh, konsole
is fat" and then turn to their forum of choice and lament how terribly
inefficient our software is.

>> Sometimes the CPU column shows nan% for new apps. Happens a lot with
>> bustle for me. In fact, sometimes I have two entries one of which is
>> nan% for all eternity https://i.imgur.com/HLavLsI.png
> 
> The lingering one is odd, but may have something to do with cgroups not 
> getting cleaned up properly. If you can reproduce and provide the output of 
> systemd-cgls that would be helpful.

│   ├─user@1000.service
│   │ ├─app.slice
│   │ │ ├─app-org.freedesktop.Bustle-c200626490444a60b898eb65b179d6f7.scope
│   │ │ │ ├─216216 bwrap --args 31 bustle
│   │ │ │ ├─216224 bwrap --args 31 bustle
│   │ │ │ └─216225 bustle
│   │ ├─flatpak-org.freedesktop.Bustle-216216.scope
│   │ │ ├─216221 bwrap --args 31 xdg-dbus-proxy --args=33
│   │ │ └─216222 xdg-dbus-proxy --args=33

Mind you, both are cleaned up on exit and the second one doesn't always
appear to begin with.

>> Also the columns in the processes page lack labels.
> 
> Gah. And applications works correctly?

Yup.

HS

Re: plasma-systemmonitor in kdereview

[Harald Sitter]

On 01.10.20 11:36, Arjen Hiemstra wrote:
> Hello,
> 
> I'd hereby like to announce that plasma-systemmonitor is in kdereview. It can 
> be found at https://invent.kde.org/plasma/plasma-systemmonitor .
> 
> plasma-systemmonitor is a new system monitor UI built with Kirigami. It makes 
> use of the ksystemstats daemon and the faces system for system monitor 
> plasmoids that were both introduced in Plasma 5.19.
> 
> Our current plan is to do a "preview release" alongside Plasma 5.20, then 
> have 
> it be an official part of Plasma with 5.21.

Cool stuff.

L10n is currently a bit incomplete.
Notably
- the pages files lack any localization at all and I'm also not sure how
those could be best localized.
- the 'configure columns' feature doesn't load the translated strings
into the checkboxes

On the subject of configure columns I feel like word wrapping would look
better than eliding there https://i.imgur.com/RbAeUhH.png

Something is astray with memory calculation. See konsole which is
somehow listed as 2.6gib https://i.imgur.com/MjIpj3O.png but its actual
children aren't getting nowhere near that number. I'm guessing it's
because even detached processes (notably vscode here) are in the same
cgroup? If so we probably can't do much about this?

The table highlights are getting messed up when new apps appear/disappear.
New app appears: https://i.imgur.com/tacWeoI.png mind that suddenly the
ksysguard memory cell is selected as well. More or less the same happens
when a process disappears. The cells seem somewhat random though Memory
is almost always a highlighted one.

Sometimes the CPU column shows nan% for new apps. Happens a lot with
bustle for me. In fact, sometimes I have two entries one of which is
nan% for all eternity https://i.imgur.com/HLavLsI.png

Possibly related: when editing the Processes page and discarding all
changes the R/W columns show negative values for a second or two.
https://i.imgur.com/amrfewG.png

Also the columns in the processes page lack labels.

HS

Re: stale MR triage

[Harald Sitter]

On 19.09.20 02:18, Albert Astals Cid wrote:
> Not really, the thing is, for Okular i knew that all things that were old 
> didn't need my attention, it was either on the patch author to come back and 
> fix something or someone else should be the one reviewing, so just looking at 
> the last updated MRs gave me an overview if something had changed that needed 
> my attention, now suddenly that's not true anymore.

Hm. I'm thinking we maybe should filter out okular from the triage in
general then. In the scenarios you describe I'd expect the triage team
to most likely write ping comments of some sort, so the MRs would jump
to the top of the list even without the help of gitlab backreference.

Thoughts?

HS

Re: stale MR triage

[Harald Sitter]

On Fri, Sep 18, 2020 at 3:09 AM Ben Cooksley  wrote:
>
> On Fri, 18 Sep 2020, 2:58 am Harald Sitter,  wrote:
>>
>> Griaß eich!
>
>
> Hi Harald,
>
>>
>> In the KF6 BOF we were chatting about merge requests not being nearly
>> as actively watched because people didn't necessarily subscribe to all
>> projects. While that is a solvable problem by asking people to kindly
>> subscribe, it got us thinking that we should have a way to deal with
>> stale MRs in general. For all projects.
>
>
> Please note that from my reading of the code this would also trigger for 
> people's personal projects as well?

It only lists !personal projects:
https://invent.kde.org/sitter/triage/-/blob/master/plugin.rb#L34 takes
care fo that.

> I assume the token it currently uses is your personal one?

Yep, Bhushan was thinking we should use a bot account, and I agree.

>>
>> So here's the proposal:
>>
>> We'll setup a new triage project (prototype at [1]; going to move)
>> that project runs a pipeline once a week that runs the existing
>> gitlab-triage tool [1] to collect all MRs that haven't received an
>> update for 2 weeks. The MRs are then dumped into an issue on the
>> triage project (ex at [2]). Anyone who is willing to help out with cat
>> herding can subscribe to that project and gets notified of these
>> auto-generated issues. We can then walk through the list of stales to
>> work out a solution for getting them moving (assign a helpful
>> reviewer, ping, review ourselves).
>>
>> Any further thoughts?
>>
>> [1] https://invent.kde.org/sitter/triage/-/pipelines
>> [2] https://gitlab.com/gitlab-org/gitlab-triage
>> [3] https://invent.kde.org/sitter/demo/-/issues/2 (feel free to deal
>> with items on this list already)
>>
>> HS
>
>
> Cheers,
> Ben

Re: stale MR triage

[Harald Sitter]

On Fri, Sep 18, 2020 at 12:36 AM Albert Astals Cid  wrote:
>
> El dijous, 17 de setembre de 2020, a les 16:57:32 CEST, Harald Sitter va 
> escriure:
> > Griaß eich!
> >
> > In the KF6 BOF we were chatting about merge requests not being nearly
> > as actively watched because people didn't necessarily subscribe to all
> > projects. While that is a solvable problem by asking people to kindly
> > subscribe, it got us thinking that we should have a way to deal with
> > stale MRs in general. For all projects.
> >
> > So here's the proposal:
> >
> > We'll setup a new triage project (prototype at [1]; going to move)
> > that project runs a pipeline once a week that runs the existing
> > gitlab-triage tool [1] to collect all MRs that haven't received an
> > update for 2 weeks. The MRs are then dumped into an issue on the
> > triage project (ex at [2]). Anyone who is willing to help out with cat
> > herding can subscribe to that project and gets notified of these
> > auto-generated issues. We can then walk through the list of stales to
> > work out a solution for getting them moving (assign a helpful
> > reviewer, ping, review ourselves).
> >
> > Any further thoughts?
>
> My default "sort MR by age" view in Okular is now unusable since there's 
> suddenly a bunch of MRs that are now 2 days old instead of 9 months old.

To clarify: that's "sort by last update", not age, right? The creation
date of an MR shouldn't change, the update date however does.

> That makes me very unhappy and more unproductive.

I'm sorry, I hadn't thought of the possibility that merely mentioning
an MR counts as an update to the MR.

> And i guess it basically breaks your code since next month those MRs are not 
> without updates for 2 weeks because you just linked to them last week?

It's not the biggest concern, ideally every week all stale MRs should
be dealt with somehow anyway, so there'd be some update which would
make them not stale. Should they become stale again the delay
increases of course, but they'll appear again after 4 weeks from the
original stale mention.

> How can we fix that?

I fear that'd need taking up with gitlab. The reason they become
updated is because linking anywhere on gitlab to any MR introduces a
backreference "so and so mentioned this MR in issue #3" which as we've
found out is considered an update to the MR thus messing with the
last-update sorting. It really is not the most reasonable behavior
IMHO.

Would sorting by creation instead work for you?

HS

stale MR triage

[Harald Sitter]

Griaß eich!

In the KF6 BOF we were chatting about merge requests not being nearly
as actively watched because people didn't necessarily subscribe to all
projects. While that is a solvable problem by asking people to kindly
subscribe, it got us thinking that we should have a way to deal with
stale MRs in general. For all projects.

So here's the proposal:

We'll setup a new triage project (prototype at [1]; going to move)
that project runs a pipeline once a week that runs the existing
gitlab-triage tool [1] to collect all MRs that haven't received an
update for 2 weeks. The MRs are then dumped into an issue on the
triage project (ex at [2]). Anyone who is willing to help out with cat
herding can subscribe to that project and gets notified of these
auto-generated issues. We can then walk through the list of stales to
work out a solution for getting them moving (assign a helpful
reviewer, ping, review ourselves).

Any further thoughts?

[1] https://invent.kde.org/sitter/triage/-/pipelines
[2] https://gitlab.com/gitlab-org/gitlab-triage
[3] https://invent.kde.org/sitter/demo/-/issues/2 (feel free to deal
with items on this list already)

HS

D28855: put minimumkeepsize actually in the netpref KCM

[Harald Sitter]

sitter abandoned this revision.
sitter added a comment.


  moved to https://invent.kde.org/frameworks/kio/-/merge_requests/123

REPOSITORY
  R241 KIO

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

To: sitter, ngraham, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

Re: plasma-disks in kdereview

[Harald Sitter]

It's been 3 weeks so I'm considering it in good enough shape for
moving to plasma.

Thanks! :)

On Tue, Aug 4, 2020 at 3:46 PM Harald Sitter  wrote:
>
> Hey
>
> It'd be awesome if I could get some eyes at the new
> https://invent.kde.org/system/plasma-disks which implements smart
> monitoring as requested in https://bugs.kde.org/show_bug.cgi?id=254313
> I've opted to not put it in kinfocenter directly because it is a bit
> much with its own kded module and kauth helper, plus I guess there's
> an opportunity to add further monitoring tech for RAIDs or something.
> It still targets plasma releases though.
>
> This runtime depends on smartctl (from smartmontools) to read the
> smart status via a kauth helper. Device states are managed inside a
> kded that maps the devices into dbus from which the kinfocenter KCM
> then loads the data for visualization.
>
> Ta
> HS

Re: Extend metainfo.yaml files with License Information

[Harald Sitter]

On Mon, Aug 17, 2020 at 8:24 PM Andreas Cord-Landwehr
 wrote:
>
> Thanks! I will answer inline:
>
> On Montag, 17. August 2020 17:47:40 CEST Harald Sitter wrote:
> [...]
> > > **First question:** Shall we only list ONE or ALL licenses, same for the
> > > license information overview that should be on api.kde.org?
> >
> > The primary use would be api.kde.org, no? A third party looks for a
> > solution to hardware shenanigans with Qt and finds the solid docs and
> > the solid docs say "you may use this thingy under LGPL-2.1". If so,
> > then surely we ought to encode all artifacts and their licensing
> > terms. What's more,. the artifact a given class belongs to becomes
> > relevant and I guess that's a bit tricky to sort out.
>
> api.kde.org will be the first user, but I already see more in the pipeline:
> - for this plausibility tooling 
> <https://invent.kde.org/frameworks/extra-cmake-modules/-/merge_requests/21> I 
> would love to get a connection of the
> CMake target name and the stated outbound license
> - for Yocto recipes we already have to state the licenses by hand and it
> should not be too hard to generate the Yocto information from a Yaml file
> - in the long term I expect that this is what is useful for packages, who
> currently mostly look at the source files and have to guess
>
> > > Now, I am wondering about the best approach to encode something like this
> > > into the metainfo.yaml. I am currently considering a structure as
> > > follows:
> > >
> > > Attica's metainfo.yaml
> > > [...]
> > >
> > > outboundLicenses:
> > > libattica:
> > > - LGPL-2.1-only
> > > - LGPL-3.0-only
> > >
> > > Baloo's metainfo.yaml
> > > [...]
> > >
> > > outboundLicenses:
> > > libbaloo:
> > > - LGPL-2.1-only
> > > - LGPL-3.0-only
> > >
> > > baloo-kioslave:
> > > - GPL-2.0-only
> >
> > Why not actually use a SPDX expression? `LGPL-2.1-only OR LGPL-3.0-only`.
> >
> > Some additional concerns that come to mind:
> > - what 's the actual artifact name? for libraries we already have a
> > target name so I guess we might just use that so for example that'd be
> > KF5::Baloo. What about plugins or binaries though?
> > - how would we make sure all artifacts are encoded? do we want to even?
>
> Ouch, yes, the obvious choice, no idea why I did not see that by myself :)
> Yes, SPDX expressions should be the obvious way to go IMO. For the questions:
> - for libraries, I agree that the target name should be easiest
> - for plugins, we could use the library name as it is still a shared object
> - for applications, we could use the executable name
> - for anything that is "not changed but only installed" during the compile/
> install steps, I would say for now that those are out of scope
> In my understanding, we should strive for encoding all artifacts, but if we
> miss some we do not state something wrong. Moreover, what we state there
> should be covered by automated tests (see link above).

It all sounds reasonable to me.

I'm pipedreaming a bit ... but given the ultimate goal of stringing
artifacts to licenses maybe a thing to consider is to actually encode
this stuff as cmake properties on the actual cmake targets. Perhaps
not as a first step, but as a longer term goal. The cmake targets
already know the output name of their artifact is (resolving the
what-do-we-call-it problem), they also know which sources contribute
to a target (improving the context capabilities for api.kde.org + the
plausability tooling could actually look at the targets rather than
the explicit list of files which of course are subject to human
error).

https://cmake.org/cmake/help/latest/command/define_property.html#command:define_property
https://cmake.org/cmake/help/latest/prop_tgt/SOURCES.html

HS

Re: Extend metainfo.yaml files with License Information

[Harald Sitter]

On Mon, Aug 17, 2020 at 2:16 PM Andreas Cord-Landwehr
 wrote:
>
> Hi, I am currently looking into extending our metainfo.yaml files to provide
> information about the outbound licenses of the artefacts that are provided by
> a framework. Here a few examples:
>
> Attica: Provides libattica, which is legally OK to be used as LGPL-2.1 or
> LGPL-3.0 (and of course also as GPL-2.0 or GPL-3.0).
>
> Baloo, which is quite complicated: The library is (supposed to be; there are a
> few license issues here at the moment) LGPL-2.1 and may also be used as
> LGPL-3.0. Moreover, there is the baloo-kioslave that is GPL, then there are a
> few tools balooctl, balooshow and baloosearch, which are all GPL-2.0 or
> GPL-3.0.
>
> **First question:** Shall we only list ONE or ALL licenses, same for the
> license information overview that should be on api.kde.org?

The primary use would be api.kde.org, no? A third party looks for a
solution to hardware shenanigans with Qt and finds the solid docs and
the solid docs say "you may use this thingy under LGPL-2.1". If so,
then surely we ought to encode all artifacts and their licensing
terms. What's more,. the artifact a given class belongs to becomes
relevant and I guess that's a bit tricky to sort out.

> Now, I am wondering about the best approach to encode something like this into
> the metainfo.yaml. I am currently considering a structure as follows:
>
> Attica's metainfo.yaml
> [...]
> outboundLicenses:
> libattica:
> - LGPL-2.1-only
> - LGPL-3.0-only
>
> Baloo's metainfo.yaml
> [...]
> outboundLicenses:
> libbaloo:
> - LGPL-2.1-only
> - LGPL-3.0-only
> baloo-kioslave:
> - GPL-2.0-only

Why not actually use a SPDX expression? `LGPL-2.1-only OR LGPL-3.0-only`.

Some additional concerns that come to mind:
- what 's the actual artifact name? for libraries we already have a
target name so I guess we might just use that so for example that'd be
KF5::Baloo. What about plugins or binaries though?
- how would we make sure all artifacts are encoded? do we want to even?

HS

D7563: Add privilegeExecution field to file protocol description

[Harald Sitter]

sitter added a comment.


  In D7563#676151 , @cblack wrote:
  
  > In D7563#674682 , @sitter wrote:
  >
  > > This really cannot land right now IMHO. Dolphin can actually deadlock 
itself because it uses way too much nested event looping and will be entirely 
unresponsive to mouse inputs when certain timers happen to trigger. A trivial 
way to reproduce this is to try and duplicate a file in file:/
  >
  >
  > Can't reproduce, duplication works fine.
  
  
  It no longer deadlocks, it still doesn't work though. Now I get an empty file
  
dolphin(137764)/(kf5.kio.core.copyjob) KIO::copyAs: src= 
QUrl("file:///list") dest= QUrl("file:///list copy")
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::slotStart: 
CopyJob: stating the dest QUrl("file:///")
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJob::slotResult: d->state= 1
dolphin(137764)/(kf5.kio.core.copyjob) 
KIO::CopyJobPrivate::slotResultStating: 
dolphin(137764)/(kf5.kio.core.copyjob) 
KIO::CopyJobPrivate::slotResultStating: dest is dir: true
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::statCurrentSrc: 
fast path! found info about QUrl("file:///list") in KCoreDirLister
dolphin(137764)/(kf5.kio.core.copyjob) 
KIO::CopyJobPrivate::addCopyInfoFromUDSEntry: fileName= "list" url= 
QUrl("file:///list")
dolphin(137764)/(kf5.kio.core.copyjob) 
KIO::CopyJobPrivate::addCopyInfoFromUDSEntry: uSource= QUrl("file:///list") 
uDest(1)= QUrl("file:///list copy")
dolphin(137764)/(kf5.kio.core.copyjob) 
KIO::CopyJobPrivate::addCopyInfoFromUDSEntry:  uDest(2)= QUrl("file:///list 
copy")
dolphin(137764)/(kf5.kio.core.copyjob) 
KIO::CopyJobPrivate::addCopyInfoFromUDSEntry:   QUrl("file:///list") -> 
QUrl("file:///list copy")
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::sourceStated: 
Source is a file (or a symlink), or we are linking -> no recursive listing
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::statNextSrc: 
Setting m_dest to QUrl("file:///list copy")
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::statCurrentSrc: 
Stating finished. To copy: 39549 , available: 155277156352
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::copyNextFile: 
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::copyNextFile: 
preparing to copy QUrl("file:///list") 39549 155277156352
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::copyNextFile: 
copying "/list copy"
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::copyNextFile: 
Copying QUrl("file:///list") to QUrl("file:///list copy")
dolphin(137764)/(kf5.kio.core.copyjob) 
KIO::CopyJobPrivate::slotProcessedSize: 39549
dolphin(137764)/(kf5.kio.core.copyjob) 
KIO::CopyJobPrivate::slotProcessedSize: emit processedSize 39549
kio_file(137793)/(kf5.kio.kio_file) FileProtocol::copy: Could not change 
permissions for "/list copy"
kio_file(137793)/(kf5.kio.kio_file) FileProtocol::copy: Couldn't preserve 
group for "/list copy"
kio_file(137793)/(kf5.kio.kio_file) FileProtocol::copy: Couldn't preserve 
access and modification time for "/list copy"
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJob::slotResult: d->state= 6
dolphin(137764)/(kf5.kio.core.copyjob) 
KIO::CopyJobPrivate::slotResultCopyingFiles: 0 files remaining
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::copyNextFile: 
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::copyNextFile: 
copyNextFile finished
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJob::emitResult: 
KDirNotify'ing FilesAdded QUrl("file:///")

REPOSITORY
  R241 KIO

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

To: cblack, #frameworks, dfaure, chinmoyr, sitter, ngraham
Cc: cblack, sitter, kkong, kde-frameworks-devel, feverfew, mreeves, mati865, 
ngraham, elvisangelaccio, LeGast00n, michaelh, bruns

D7563: Add privilegeExecution field to file protocol description

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> file_unix.cpp:1372
> +break;
> +default:
> +Q_UNREACHABLE();

I'd advise handling default cases. The compiler can no longer warn of unhandled 
enum values when default is used. Instead I'd convert the entire switch into a 
helper function `static QString actionTypeToString(ActionType action)` and in 
there switch like so:

  switch (action) {
  case ActionType::CHMOD:
  return QStringLiteral("Authentication is required to change this file's 
permissions.");
  case ActionType::CHOWN:
  ...
  }
  // any values not explicitly handled gets here making this the de-facto 
default handling
  Q_UNREACHABLE()
  return QString()
  }

This then makes the code here less repetitive as the entire switch gets 
squashed down to `details.insert(KAuth::Action::AuthDetail::DetailMessage, 
actionTypeToString(action));`

REPOSITORY
  R241 KIO

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

To: cblack, #frameworks, dfaure, chinmoyr, sitter, ngraham
Cc: cblack, sitter, kkong, kde-frameworks-devel, feverfew, mreeves, mati865, 
ngraham, elvisangelaccio, LeGast00n, michaelh, bruns

Re: plasma-disks in kdereview

[Harald Sitter]

On Wed, Aug 5, 2020 at 12:00 AM Albert Astals Cid  wrote:
>
> El dimarts, 4 d’agost de 2020, a les 15:46:20 CEST, Harald Sitter va escriure:
> > Hey
> >
> > It'd be awesome if I could get some eyes at the new
> > https://invent.kde.org/system/plasma-disks which implements smart
> > monitoring as requested in https://bugs.kde.org/show_bug.cgi?id=254313
> > I've opted to not put it in kinfocenter directly because it is a bit
> > much with its own kded module and kauth helper, plus I guess there's
> > an opportunity to add further monitoring tech for RAIDs or something.
> > It still targets plasma releases though.
> >
> > This runtime depends on smartctl (from smartmontools) to read the
> > smart status via a kauth helper. Device states are managed inside a
> > kded that maps the devices into dbus from which the kinfocenter KCM
> > then loads the data for visualization.
>
> I have a feeling that the qml i18n's won't work pick up the correct domain, 
> the -DTRANSLATION_DOMAIN only sets the domain for the C++ calls.

Good point. qml extraction was missing too.
As it turns out the qml kcms get their domain name from the kaboutdata
componentname, so I rejiggered the kcm accordingly and the x-test l10n
now works. I do wonder if it'd make more sense to make the kcm a
separate translation domain altogether, calling everything
plasma_disks feels a bit awkward. At the same time having 2
translation domains for 10 strings seems silly too :S

HS

plasma-disks in kdereview

[Harald Sitter]

Hey

It'd be awesome if I could get some eyes at the new
https://invent.kde.org/system/plasma-disks which implements smart
monitoring as requested in https://bugs.kde.org/show_bug.cgi?id=254313
I've opted to not put it in kinfocenter directly because it is a bit
much with its own kded module and kauth helper, plus I guess there's
an opportunity to add further monitoring tech for RAIDs or something.
It still targets plasma releases though.

This runtime depends on smartctl (from smartmontools) to read the
smart status via a kauth helper. Device states are managed inside a
kded that maps the devices into dbus from which the kinfocenter KCM
then loads the data for visualization.

Ta
HS

Re: Policy on forward declarations for things from external libraries

[Harald Sitter]

On Fri, Jul 31, 2020 at 5:56 PM Vlad Zahorodnii  wrote:
>
> Howdy,
>
>  From time to time, I find myself in a situation where a code reviewer
> suggests to replace #include  with the corresponding class
> forward declaration. Such discussions usually get us nowhere because
> neither the code reviewer nor I mind to seek for a compromise. A policy
> would prevent wasting time on such discussions.
>
> Note that I used Qt/QFoobar only as an example. Similar logic can be
> applied to other libraries as well. For example, libxcb or libexif, etc.
>
> The good thing about forward declarations is that they can reduce the
> compilation time quite significantly.
>
> The bad thing about it is that we must guarantee that the definition of
> QFoobar in the external library won't change. For example, QFoobar won't
> move into an inline namespace all of sudden, etc.
>
> I propose to add a paragraph in [1] saying whether it is okay to add
> forward declarations for things from external libraries.
>
> Any thoughts or suggestions?

IMHO it'd only blow up the policy. This has more to do with API design
than with forward declaration. As such this is kind of covered by the
policy on d-pointers and Qt's policy on being minimal. Most API will
not want any headers or objects other than Qt, cpp and libc in
*public* headers anything beyond that very very very likely belongs in
a Private class. In particular when you have reason to believe that
the authors of the library may not be able to keep their compatibility
for as long as we do or to the degree we do. Or put the other way
around: if the upstream's compatibility promise isn't at least as good
as our own then it has no business being part of our public API,
forward decl or not.

With that in mind the existing policy on forward decls is fairly clear
on the matter: use forward decls whenever possible. Forward declaring
has very practical advantages that are always worth having.

HS

Re: xml_mimetypes5 and kcoreaddons

[Harald Sitter]

On Sun, Jul 19, 2020 at 1:56 PM David Faure  wrote:
>
> On mercredi 15 juillet 2020 12:24:34 CEST Harald Sitter wrote:
> > On Wed, Jul 15, 2020 at 12:39 AM David Faure  wrote:
> > > On mardi 14 juillet 2020 19:35:41 CEST Albert Astals Cid wrote:
> > > > El dimarts, 14 de juliol de 2020, a les 15:14:38 CEST, Jonathan Riddell
> > > > va
> > >
> > > escriure:
> > > > > We're playing with translations in neon packages and looking at
> > > > > kcoreaddons
> > > > > the tars have
> > > > > xml_mimetypes5
> > > > > But we can't see anything in the code which uses this.  Do these
> > > > > translations get used?
> > > >
> > > > Yes, the translations are used.
> > > >
> > > > No, they don't need to be on the tarball.
> > > >
> > > > The translations are used at scripty time to to fill
> > > > https://invent.kde.org/frameworks/kcoreaddons/-/blob/master/src/mimetype
> > > > s/k
> > > > de5.xml
> > > >
> > > > Luigi recently did a change so the files ending in _xml_mimetypes.po
> > > > don't
> > > > get added to the release service tarballs.
> > > >
> > > > This didn't work here because a) KF5 is using a different branch of
> > > > release-tools b) the file doesn't follow the same naming pattern than
> > > > the
> > > > rest of xml mimetype files we have.
> > > >
> > > > If DavidF is fine adapting his scripts to not release files ending in
> > > > _xml_mimetypes.po (i.e. him or someone else patches the scripts) i
> > > > volunteer to do the small patch for src/mimetypes/XmlMessages.sh to
> > > > rename
> > > > it.
> > >
> > > Fine with me, but don't we have more cases of the same kind, with
> > > different
> > > names? Any case of translations being "integrated" into some file leads to
> > > this. Desktop files, mimetype XML, is there really nothing else?
> >
> > It's a bit more "fluid" than one would hope. Looking at
> > update_translations the following patterns exist:
> >
> > - created by XmlMessages.sh unfortunately arbitrary named files but
> > largely using the standard suffix Albert mentioned (the only other
> > violation I can find with lxr is kpat)
> > - created by StaticMessages.sh also arbitrary, only used for websites
> > I think (?)
> > - ._desktop_.po automatic - shouldn't be shipped
> > - ._json_.po automatic - shouldn't be shipped
> > - .appdata.po automatic - shouldn't be shipped
> > - .metadata.po automatic - shouldn't be shipped
>
> OK. make_rc_tag.sh already says
>
> find -regextype egrep -regex '.*\.(_desktop_|_json_|appdata|metainfo)\.po' 
> -delete
>
> which matches the above, assuming you meant metainfo when writing metadata?

Indeed that's what I meant :)

HS

Re: xml_mimetypes5 and kcoreaddons

[Harald Sitter]

On Wed, Jul 15, 2020 at 12:39 AM David Faure  wrote:
>
> On mardi 14 juillet 2020 19:35:41 CEST Albert Astals Cid wrote:
> > El dimarts, 14 de juliol de 2020, a les 15:14:38 CEST, Jonathan Riddell va
> escriure:
> > > We're playing with translations in neon packages and looking at
> > > kcoreaddons
> > > the tars have
> > > xml_mimetypes5
> > > But we can't see anything in the code which uses this.  Do these
> > > translations get used?
> >
> > Yes, the translations are used.
> >
> > No, they don't need to be on the tarball.
> >
> > The translations are used at scripty time to to fill
> > https://invent.kde.org/frameworks/kcoreaddons/-/blob/master/src/mimetypes/k
> > de5.xml
> >
> > Luigi recently did a change so the files ending in _xml_mimetypes.po don't
> > get added to the release service tarballs.
> >
> > This didn't work here because a) KF5 is using a different branch of
> > release-tools b) the file doesn't follow the same naming pattern than the
> > rest of xml mimetype files we have.
> >
> > If DavidF is fine adapting his scripts to not release files ending in
> > _xml_mimetypes.po (i.e. him or someone else patches the scripts) i
> > volunteer to do the small patch for src/mimetypes/XmlMessages.sh to rename
> > it.
>
> Fine with me, but don't we have more cases of the same kind, with different
> names? Any case of translations being "integrated" into some file leads to
> this. Desktop files, mimetype XML, is there really nothing else?

It's a bit more "fluid" than one would hope. Looking at
update_translations the following patterns exist:

- created by XmlMessages.sh unfortunately arbitrary named files but
largely using the standard suffix Albert mentioned (the only other
violation I can find with lxr is kpat)
- created by StaticMessages.sh also arbitrary, only used for websites
I think (?)
- ._desktop_.po automatic - shouldn't be shipped
- ._json_.po automatic - shouldn't be shipped
- .appdata.po automatic - shouldn't be shipped
- .metadata.po automatic - shouldn't be shipped

(of course also Messages.sh with arbitrary naming, but that's the
regular translations so they need shipping ^^)

HS

Re: CMake source files without license

[Harald Sitter]

On Mon, Jun 22, 2020 at 11:27 PM Elvis Angelaccio
 wrote:
>
> Hi,
>
> I was wondering, is there a reason why our CMakeLists.txt files often do
> not contain a license?

The majority are probably trivial works that nobody can really claim
copyright on and to that end cannot extend a license either.

> Shouldn't they have one? Or at least the CMakeLists.txt files that are
> "big" enough?

That is of course the trouble. When does a cmakelists file become a
copyrightable work that needs a license to be made free?

With that in mind perhaps we should simply make it a matter of policy
that all new cmakelists should be equipped with spdx tags? Always,
even if they only contain a single line. If all files have a license
specified we at least can't forget to add one later and everyone is on
the same page about the license, should (part of) a cmakelists become
a Finder or otherwise derived work later.
For existing files one could opt to explicitly license them as needed.
The unfortunate thing there is that at least for moral reasons one
needs to ask all prior authors for consent, and that's a fair amount
of work.

HS

D7563: Add privilegeExecution field to file protocol description

[Harald Sitter]

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


  This really cannot land right now IMHO. Dolphin can actually deadlock itself 
because it uses way too much nested event looping and will be entirely 
unresponsive to mouse inputs when certain timers happen to trigger. A trivial 
way to reproduce this is to try and duplicate a file in file:/
  
  Also making folders is actually not even implemented as the relevant mkdirjob 
seems to lack privilege enablement. That's not strictly speaking blocking but 
renders the actual UX broken.
  I've started a page for more comprehensive testing of the entire polkit epic.
  https://invent.kde.org/frameworks/kio/-/wikis/PrivilegedOperations
  
  I understand this diff is the last piece missing to enable the entire 
priviledge operations systems, so the entire shebang better be working first (:

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, dfaure, chinmoyr, sitter
Cc: sitter, kkong, kde-frameworks-devel, feverfew, mreeves, mati865, ngraham, 
elvisangelaccio, LeGast00n, cblack, michaelh, bruns

D29745: look for kded as runtime dep

[Harald Sitter]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:15869b83200f: look for kded as runtime dep (authored by 
sitter).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29745?vs=82839=83201

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

AFFECTED FILES
  CMakeLists.txt

To: sitter, dfaure, apol, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

Re: Recent breakage in kwallet

[Harald Sitter]

here's my understanding of the problem:

firstly, this is likely only a problem on debian-derived systems. and
also only with blowfish

debian has this patch [1] which on *actual* little endian systems
(x86,arm64...) would run the conditional branches. the thing to note
is that not only QBO is defined but also the ifs get changed. so, on
LE systems that original patch was behaving the same as the QBO==QBE
check (with both undefined) in kwallet proper. on BE systems however
the conditional aren't run and so the output is different between us
(kf5) and them (debian-derived system).
so far so messy.

what I think was the trouble Marco mentioned is that the original
debian patch was badly merged with the change David made. specifically
because of the changed conditionals, QBO must not be hardcoded with
that patch as that'd make QBO!=QLE and fail the conditionals below
thus change behavior.

in short: I don't think David's commit broke anything. I've also made
a potential fix for the actual bug in neon just now.

also about [2] I'm sure that's not quite right because it breaks
things the other way around (i.e. for it to work it'd also have to
change the conditionals from QBO==QBE to QBO==QLE, same as the debian
patch). and that would technically break wallets on big endian
architectures (not that I'm sure we have many users on those). the way
I see it kwallet can never be properly endian aware as it'd break
legacy compatibility. equally debian can never not have the patch
because it'd then no longer be properly endian aware and break its
legacy compatibility well, of course either could decide that big
endian isn't important enough and either become endian aware, or
endian unaware ;)

[1] 
https://packaging.neon.kde.org/kde/kwallet.git/commit/debian/patches/blowfish_endianess.diff?id=188ca124c69902e4fba01d2af6b419e1aac71177
[2] https://invent.kde.org/frameworks/kwallet/-/merge_requests/1

D29381: Thumbnail text: use libmagic to detect encoding

[Harald Sitter]

sitter added a comment.


  Browsing the code it looks like it mmaps the file though? And when I add some 
strategic sleeping I can verify that file goes towards shared memory.
  
  Oh, I suppose the trouble is that load gets called each ::create? Simply wrap 
it in a cpp class and static scope it to something maybe?
  
  The finder by the way is still in disarray I've noticed. Please look at one 
of the more modern finds in ECM for reference. Also I think the find_library 
call is wrong, it should look for 'magic' not 'libmagic'
  
Each library name given to the NAMES option is first considered as a 
library file name and then considered with platform-specific prefixes (e.g. 
lib) and suffixes (e.g. .so).
  
  i.e. either the NAME is `libmagic.so` (which is a file) or `magic` (which 
gets platform adjusted), but probably not `libmagic` as that is neither a file 
nor will the platform adjusted `liblibmagic.so` be one.

REPOSITORY
  R320 KIO Extras

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

To: meven, #frameworks, sitter, ngraham
Cc: pino, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29381: Thumbnail text: use libmagic to detect encoding

[Harald Sitter]

sitter added a comment.


  LGTM. Seeing as I don't have much background knowledge I'm not comfortable 
accepting though. I guess if nobody comes up with better options by next week 
feel free to land.

REPOSITORY
  R320 KIO Extras

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

To: meven, #frameworks, sitter, ngraham
Cc: pino, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29743: sftp: map sftp_open error to kio error

[Harald Sitter]

This revision was automatically updated to reflect the committed changes.
Closed by commit R320:07e44cb1b536: sftp: map sftp_open error to kio error 
(authored by sitter).

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29743?vs=82835=82903

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

AFFECTED FILES
  sftp/kio_sftp.cpp

To: sitter, feverfew
Cc: kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29745: look for kded as runtime dep

[Harald Sitter]

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

REVISION SUMMARY
  kded is called over dbus to talk to the proxyscout, and also to track
  uidelegate windows it seems.
  this codifies this relationship on a cmake level allowing distributions
  and user to actually be aware.

TEST PLAN
  reported as RUNTIME package in cmake summary

REPOSITORY
  R241 KIO

BRANCH
  kded

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

AFFECTED FILES
  CMakeLists.txt

To: sitter, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29743: sftp: map sftp_open error to kio error

[Harald Sitter]

sitter created this revision.
sitter added a reviewer: feverfew.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.
sitter requested review of this revision.

REVISION SUMMARY
  sftp gives out more relevant errors such as SSH_FX_PERMISSION_DENIED,
  let's forward them as KIO errors instead of using the general cannot open
  error.

TEST PLAN
  hoping for the best! bad permissions on the remote now actually raise 
suitable kio errors, functionally that seems to make little difference 
unfortunately

REPOSITORY
  R320 KIO Extras

BRANCH
  release/20.04

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

AFFECTED FILES
  sftp/kio_sftp.cpp

To: sitter, feverfew
Cc: kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Harald Sitter]

This revision was automatically updated to reflect the committed changes.
Closed by commit R320:1df6174834bb: sftp: break large writes into multiple 
requests (authored by sitter).

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29634?vs=82723=82834

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

AFFECTED FILES
  sftp/kio_sftp.cpp
  sftp/kio_sftp.h

To: sitter, ngraham, meven, feverfew
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29526: Thumbnails: make thumbnail generation dpr-aware

[Harald Sitter]

sitter added a comment.


  You do seem to calculate the deviceWidth and height an awful lot, it makes 
reading a bit clunky. I'd much rather have the multiplication done once and 
then always use the var instead. In fact, perhaps it'd make sense to have 
createV3 feed the values into the implementations? Currently they all repate 
the same two lines over and over.

INLINE COMMENTS

> djvucreator.cpp:61
>QByteArray sizearg, fnamearg;
> -  sizearg = QByteArray::number(width) + 'x' + QByteArray::number(height);
> +  sizearg = QByteArray::number(width * devicePixelRatio) + 'x' + 
> QByteArray::number(height * devicePixelRatio);
>fnamearg = QFile::encodeName( path );

Surely multiplication results need converting to int. That being said, you do 
multiply below again, so maybe just move maxWidth/Height up here.

> thumbnail.cpp:774
>  
>  void ThumbnailProtocol::scaleDownImage(QImage& img, int maxWidth, int 
> maxHeight)
>  {

Var naming is a bit inconsistent across the code base now. In the 
implementations there are maxWidth/H that are device-adjusted but in here they 
are not. Not a huge concern, just noticed.

REPOSITORY
  R320 KIO Extras

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

To: meven, #frameworks, dfaure, broulik, sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Harald Sitter]

sitter edited the test plan for this revision.

REPOSITORY
  R320 KIO Extras

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

To: sitter, ngraham, meven
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Harald Sitter]

sitter updated this revision to Diff 82723.
sitter added a comment.


  - rejigger write segmentation into new sftpWrite function used by both the 
put() and write()
  - fix length calculation
  - refine buf size comment

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29634?vs=82534=82723

BRANCH
  release/20.04

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

AFFECTED FILES
  sftp/kio_sftp.cpp
  sftp/kio_sftp.h

To: sitter, ngraham, meven
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> meven wrote in kio_sftp.cpp:58
> Why not change it now to 32 * 1024 then ?
> I guess you tested this value works at least  with openssh.
> 
> I guess the best solution would be to ask/figure out the server buffer size.
> 
> What does gvfs, or other libs ?

The reason I am not changing it is because it is out of scope for the bug fix 
and requires research. I'm also kinda leaning towards keeping it until there's 
a bug report for a server that doesn't support it. The way the RFC is written 
reads incredibly open ended to the point where there may be no maximum or it 
may be even smaller than 32kb.

> feverfew wrote in kio_sftp.cpp:1831-1832
> I'm not sure if that would help. `MAX_XFER_BUF_SIZE` would be the upper bound 
> of this approach, and if the server buffer size is less I imagine we'll crash 
> anyway (as detailed in the bug report). If we simply write less then yes, we 
> can use this to "calibrate" but seeming as it crashes instead then 
> unfortunately I don't think this will work.

What Alex said.

I guess we could try to infer if a write failed because of buffer size but it 
seems a waste of time in the grand scheme of things (and also has lots of pits 
to fall into since we'd have to hook onto a callback and then carry out a 
mid-flight session reset).

In the long run we aren't loosing much by going with a static size and request 
queuing like we do for read() already. From what I have seen in the past the 
small requests of sftp become largely irrelevant with queuing and efficient 
(threaded) encryption.

REPOSITORY
  R320 KIO Extras

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

To: sitter, ngraham, meven
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Harald Sitter]

sitter created this revision.
sitter added a reviewer: ngraham.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.
sitter requested review of this revision.

REVISION SUMMARY
  servers have arbitrary limits that we should stay below. to ensure this
  happens use MAX_XFER_BUF_SIZE as maximum size per request. if we read
  data large than that, break it apart into multiple requests
  
  (I am mostly guessing here, the rfc doesn't seem to impose any size
   constraint on the write requests themselves, so probably packet
   constraints apply by default)
  
  BUG: 404890
  FIXED-IN: 20.04.2

TEST PLAN
  - fallocate -l 128M file
  - sftp to localhost
  - copy file from one dir to another

REPOSITORY
  R320 KIO Extras

BRANCH
  release/20.04

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

AFFECTED FILES
  sftp/kio_sftp.cpp

To: sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29461: Fix kio-extras build on Windows

[Harald Sitter]

sitter added a comment.


  Pid changes look fine, though perhaps we should just throw those two lines 
away? With Qt5 logging the pid is fairly pointless because one can simply set 
QT_MESSAGE_PATTERN to include the pid when necessary 
https://doc.qt.io/qt-5/qtglobal.html#qSetMessagePattern

INLINE COMMENTS

> CMakeLists.txt:26
> Qt5::Network
> -   ssh)
> +   ${LIBSSH_LIBRARIES})
>  set_target_properties(kio_sftp PROPERTIES OUTPUT_NAME "sftp")

Hm, I am a bit hazy on the details but I think this changes makes no sense. 
libssh (upstream) introduced an imported target `ssh`. For backwards 
compatibility we also inject this target when building with older libssh's than 
the latest (to be honest though, with libssh you basically always want the 
latest or you'll have an incredibly subpar experience).

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, vonreth, meven
Cc: sitter, meven, kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, 
feverfew, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, 
bruns, emmanuelp, rdieter, mikesomov

D29381: Thumbnail text: use libmagic to detect encoding

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> meven wrote in textcreator.cpp:38
> Without libmagic, it is current state basically UTF-8 with bom detection 
> otherwise local codec.
> 
> I did not test exhaustive encodings so I wanted to let the door open for 
> users to not rely on libmagic.
> libmagic works well from what I've tested but I could not be absolutely sure 
> for the multiple encodings out there.
> Hopefully libmagic does a better job detecting UTF-8 (which I saw) but for 
> users not using much UTF-8...
> 
> And libmagic loads a 5M file storing its heuristics each time it loads ( 
> /usr/share/misc/magic.mgc ).
> It would be great to keep this in memory somewhere, maybe a static.

Perhaps it'd make sense to refactor this a bit and construct some test cases 
around encoding detection so we get a sense of reliablity?

The way I am looking at this: either libmagic always does the best job at 
detecting encodings, at which point we'll want it as a required dep, or there's 
something better in which case we don't want libmagic at all and instead use 
the something better ;)

In the end the user isn't necessarily in charge of what a random file will be 
encoded with, so I don't think there's a point in letting the user (or the 
distro) build an inferior product by accidentally not including libmagic. The 
truth is neither we nor the user can with any certainty say what encodings the 
thumbnailer will encounter.

REPOSITORY
  R320 KIO Extras

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

To: meven, #frameworks, sitter, ngraham
Cc: pino, kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, 
aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, rdieter, mikesomov

D29381: Thumbnail text: use libmagic to detect encoding

[Harald Sitter]

sitter added a comment.


  I have zero background knowledge here, but it really feels like there must be 
an existing solution to this problem other than libmagic. Like how does kate 
figure out the encoding of a text file.

INLINE COMMENTS

> Findlibmagic.cmake:1
> +# - Try to find libssh
> +# Once done this will define

bad copypaste

> Findlibmagic.cmake:64
> +
> +
> +endif (LIBMAGIC_INCLUDE_DIR AND LIBMAGIC_LIBRARIES)

It's more idiomatic to define an imported target, see some of the newer finders 
in ECM.

I do also rather wonder if we couldn't just use FindPkgConfig's imported 
target, in theory pkgconfig also works on windows. I may well be ignorant of 
the finer points of windows support though.

> textcreator.cpp:38
> +#include 
> +#if LIBMAGIC_FOUND
> +  #include "magic.h"

TBH, I would make libmagic required for building the thumbnail plugin. I can't 
see much of a rationale for why we'd want to support "broken"/insufficient 
encoding detection when there's code that makes it better.

> textcreator.cpp:65
> +#if LIBMAGIC_FOUND
> +static QTextCodec* codecFromFile(const QString )
> +{

`*` on wrong side of space

> textcreator.cpp:67
> +{
> +magic_t m = magic_open(MAGIC_MIME_ENCODING);
> +magic_load(m, nullptr);

better name than m? (:

> textcreator.cpp:69
> +magic_load(m, nullptr);
> +const char *ret = magic_file(m, path.toLocal8Bit() );
> +auto codecName = QString(ret).toUpper().toLocal8Bit();

excess whitespace towards the end. I also wonder if qfile::encodename would be 
better

> textcreator.cpp:70
> +const char *ret = magic_file(m, path.toLocal8Bit() );
> +auto codecName = QString(ret).toUpper().toLocal8Bit();
> +magic_close(m);

I guess you could just toUpper on a QBA instead of going through a temporary 
QString since ret is an encoding identifier ajnd would be always an ascii 
string.
Also, can be const it seems.

> textcreator.cpp:78
> +if (strcmp(ret, "unknown-8bit")) {
> +// use latin for unkwnwn 8bit as it is quite versatile
> +return QTextCodec::codecForName("latin-1");

unkwnwn typo

REPOSITORY
  R320 KIO Extras

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

To: meven, #frameworks, sitter, ngraham
Cc: pino, kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, 
aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, rdieter, mikesomov

D29149: Fix kio-extras compilation with -DQT_NO_CAST_TO_ASCII

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> dfaure wrote in CMakeLists.txt:60
> Isn't that what I did here? Now all of kio-extras gets that flag.
> 
> And BTW everything built by kdesrc-build actually builds with this flag 
> (since I have it in my kdesrc-buildrc). But that doesn't allow setting it in 
> ECM, there is more code using ECM out there...

Huh, I must have scrolled right past it. All is perfect!

REPOSITORY
  R320 KIO Extras

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

To: dfaure, thiago, sitter
Cc: meven, kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, 
feverfew, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, 
bruns, emmanuelp, rdieter, mikesomov

D29149: Fix kio-extras compilation with -DQT_NO_CAST_TO_ASCII

[Harald Sitter]

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


  Sure, I was referring to the smb module, not even kio-extras as a whole. ^^
  
  Anyway, diff looks reasonable.

REPOSITORY
  R320 KIO Extras

BRANCH
  master

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

To: dfaure, thiago, sitter
Cc: meven, kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, 
feverfew, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, 
bruns, emmanuelp, rdieter, mikesomov

D29149: Fix kio-extras compilation with -DQT_NO_CAST_TO_ASCII

[Harald Sitter]

sitter added a comment.


  Shouldn't we just make the entire thing QT_NO_CAST_TO_ASCII by default?

REPOSITORY
  R320 KIO Extras

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

To: dfaure, thiago, sitter
Cc: kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, 
aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, rdieter, mikesomov

D29063: Fix testpackage-appstream: XDG_DATA_DIRS needs to be explicitly extended

[Harald Sitter]

sitter added a comment.


  Looks reasonable.
  
  @apol thoughts?

REPOSITORY
  R290 KPackage

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

To: kossebau, #frameworks, mart, apol, sitter, bcooksley
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28909: smb: port to Result system to force serialization of error/finish condition

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> dfaure wrote in kio_smb.h:96
> Whenever we port a 3rd slave to it ;-)

Sure, if you think it's solid enough from an API POV.

I was thinking that we should amend the slavebase API for KF6 in general. 
Instead of having error/finished/opened all functions on an API level should 
return a Result and the slave loop would emit the relevant signal based on the 
Result. IOW: what currently happens in the derived SlaveBases actually ought to 
be KIO-internal.

That would then also allow us to get rid of the two-class split again. And the 
"fronting" class is actually a much bigger concern than Result to me. The call 
finalization logic is 100% code copy and so very easy to get wrong (e.g. sftp's 
special() not finishing when in fact it should).

> dfaure wrote in kio_smb.h:269
> parse error?

That line only moved, I am not quite sure what it is meant to tell us though. 
The header is and was quite the mess.

REPOSITORY
  R320 KIO Extras

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

To: sitter, dfaure
Cc: meven, kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, 
iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, rdieter, mikesomov

D6794: assert the testpackage appstream data validates

[Harald Sitter]

sitter added a comment.


  @bcooksley I would think the CI image needs to ship some general purpose 
schemas. install 
https://software.opensuse.org/package/gsettings-desktop-schemas I guess.
  
  If it's still not working right with that installed I can take a look at 
what's missing. The warning does sound rather generic so I have high hopes for 
having any schemas resolving it.

REPOSITORY
  R290 KPackage

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

To: sitter, sebas, apol
Cc: bcooksley, dfaure, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D28909: smb: port to Result system to force serialization of error/finish condition

[Harald Sitter]

sitter created this revision.
sitter added a reviewer: dfaure.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.
sitter requested review of this revision.

REVISION SUMMARY
  the Result system was originally introduced to the FTP slave and now also
  makes an appearance in the SMB slave. the system introduces a separation
  between logic and fronting API class to more tightly control when state
  changing calls (finished()/error()) are made. since these calls may only
  be made once during a given command multiple calls are at the very least
  indicative of bad code and at worst cause severe state confusion for the
  slavebase that it won't be able to recover from, rendering the slave
  instance broken.
  
  in the internal class Results are returned whenever an error can appear and
  these Results must be handled in some form. the only way to effectively
  produce user visible errors is to forward results up the call chain.
  
  this is also introducing scopeguards for file descriptors to simplify
  error condition returning (i.e. not having to worry about closing the fd
  manually). as a result we now require Qt 5.12 (which we do via KF5.66
  anyway, albeit implicitly)

TEST PLAN
  - copy, lots and lots of copy
  - rename
  - delete

REPOSITORY
  R320 KIO Extras

BRANCH
  smb-result

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

AFFECTED FILES
  CMakeLists.txt
  smb/kio_smb.cpp
  smb/kio_smb.h
  smb/kio_smb_auth.cpp
  smb/kio_smb_browse.cpp
  smb/kio_smb_config.cpp
  smb/kio_smb_dir.cpp
  smb/kio_smb_file.cpp
  smb/kio_smb_mount.cpp

To: sitter, dfaure
Cc: kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, 
fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, rdieter, mikesomov

D28855: put minimumkeepsize actually in the netpref KCM

[Harald Sitter]

sitter added a comment.


  Coding style is actually wildly inconsistent in that file, so I've stuck to 
what similar other lines have.

REPOSITORY
  R241 KIO

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

To: sitter, ngraham, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D28855: put minimumkeepsize actually in the netpref KCM

[Harald Sitter]

sitter added a comment.


  And didn't we have a better spinny box for byte units somewhere? Where the 
user can put `1 byte` or `1 kib` or `1 gib` and the box knows what to do?

REPOSITORY
  R241 KIO

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

To: sitter, ngraham, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28855: put minimumkeepsize actually in the netpref KCM

[Harald Sitter]

sitter added a comment.


  F8240046: Screenshot_20200415_164836.png 

  
  I'm very open for better labels, it's a horrible concept to explain in a few 
words.

REPOSITORY
  R241 KIO

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

To: sitter, ngraham, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28855: put minimumkeepsize actually in the netpref KCM

[Harald Sitter]

sitter updated this revision to Diff 80204.
sitter added a comment.


  improve label a tad

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28855?vs=80203=80204

BRANCH
  master

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

AFFECTED FILES
  src/kcms/kio/netpref.cpp
  src/kcms/kio/netpref.h

To: sitter, ngraham, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28855: put minimumkeepsize actually in the netpref KCM

[Harald Sitter]

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

REVISION SUMMARY
  I didn't even know it was a thing! by the looks of it the setting only
  applies to retention of .part files on aborting transfers (that is, files
  must be at least this size so they get retained on aborting transfers).
  so the setting is in a form inside a widget and that widget's enabled
  state is contient on partial marking being enabled
  
  CCBUG: 419987

TEST PLAN
  look at the kcm and toggle the button

REPOSITORY
  R241 KIO

BRANCH
  master

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

AFFECTED FILES
  src/kcms/kio/netpref.cpp
  src/kcms/kio/netpref.h

To: sitter, ngraham, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28679: [KPropertiesDialog] Disable changing dir icon on samba shares

[Harald Sitter]

sitter added a comment.


  Hm. So, this is a bit complicated I've noticed.
  
  Let's consider the following cases:
  
  - `desktop:` is not a local file but can set and read dir icons without 
penalty
  - `camera:` is not a local file AND `Class=:local` BUT (I think?) cannot set 
dir icons as it is `writing=false` (in practice the slave seems to list dirs 
with `rwx` though, so I'm not sure, may be a slave bug)
  - `smb:` is not a local file AND not local class nor do we want to have icons 
for it
  
  With that in mind I believe Kai's approach would be more correct here as 
otherwise the desktop: case doesn't work. Perhaps it'd make also sense to check 
`!KProtocolInfo::supportsWriting`. The way I see it if a slave implementation 
doesn't support writing... then it doesn't support writing. The directory being 
writable or not wouldn't change the lack of write support in the slave.

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28679: [KPropertiesDialog] Disable changing dir icon on samba shares

[Harald Sitter]

sitter added a comment.


  Looks reasonable. Though... Shouldn't that rather be any protocol that isn't 
`file`? Or at least all that are remote? (assuming we have a way of telling 
which slaves are remote)
  
  If I open sftp it also shows no dir icons yet lets me set one.

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, sitter
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28336: Drop klauncher usage from KCrash

[Harald Sitter]

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

INLINE COMMENTS

> kcrash.cpp:627
>  
>  void KCrash::startProcess(int argc, const char *argv[], bool waitAndExit)
>  {

This seems to serve no purpose anymore. startProcessInternal could be merged 
into this so there's only one startProcess function left.

REPOSITORY
  R285 KCrash

BRANCH
  master

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

To: davidedmundson, sitter
Cc: sitter, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28463: do not install testengine

[Harald Sitter]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:6e39c1de5bb3: do not install testengine (authored by 
sitter).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28463?vs=78974=79464

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

AFFECTED FILES
  tests/testengine/CMakeLists.txt

To: sitter, mart, apol
Cc: broulik, apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D28513: smb: use prettyname.kio-discovery-wsd for hostname of wsdiscoveries

[Harald Sitter]

This revision was automatically updated to reflect the committed changes.
Closed by commit R320:daec307b59e9: smb: use prettyname.kio-discovery-wsd for 
hostname of wsdiscoveries (authored by sitter).

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28513?vs=79234=79457

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

AFFECTED FILES
  smb/kio_smb_browse.cpp
  smb/wsdiscoverer.cpp

To: sitter, ngraham, meven
Cc: kossebau, kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, 
iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D27804: smb: add hack to support spaces in workgroup names

[Harald Sitter]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R320:f40191a147c9: smb: add hack to support spaces in 
workgroup names (authored by sitter).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D27804?vs=76934=79458#toc

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27804?vs=76934=79458

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

AFFECTED FILES
  smb/autotests/smburltest.cpp
  smb/kio_smb_browse.cpp
  smb/smburl.cpp

To: sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, thiago, azyx, nikolaik, pberestov, 
iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D27902: smb: figure out the best host to use for the UDS_URL

[Harald Sitter]

sitter abandoned this revision.

REPOSITORY
  R320 KIO Extras

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

To: sitter, ngraham, dfaure
Cc: meven, kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, 
iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, 
feverfew, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, 
bruns, emmanuelp, rdieter, mikesomov

D28513: smb: use prettyname.kio-discovery-wsd for hostname of wsdiscoveries

[Harald Sitter]

sitter updated this revision to Diff 79234.
sitter added a comment.


  chop instead of convoluted remove

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28513?vs=79220=79234

BRANCH
  smb-lazy-resolve

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

AFFECTED FILES
  smb/kio_smb_browse.cpp
  smb/wsdiscoverer.cpp

To: sitter, ngraham, meven
Cc: kossebau, kde-frameworks-devel, kfm-devel, nikolaik, pberestov, iasensio, 
fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D27504: smb faster copy to local

[Harald Sitter]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R320:46b5fb425c14: smb: fast copy (authored by sitter).

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27504?vs=77343=79233

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

AFFECTED FILES
  smb/CMakeLists.txt
  smb/autotests/CMakeLists.txt
  smb/autotests/transfertest.cpp
  smb/kio_smb.h
  smb/kio_smb_dir.cpp
  smb/kio_smb_file.cpp
  smb/transfer.cpp
  smb/transfer.h

To: sitter, ngraham, cfeck, #frameworks, #dolphin
Cc: mmustac, meven, hallas, anthonyfieroni, asturmlechner, 
kde-frameworks-devel, kfm-devel, nikolaik, pberestov, iasensio, fprice, 
LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, feverfew, michaelh, 
spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, 
mikesomov

D27504: smb faster copy to local

[Harald Sitter]

sitter edited the summary of this revision.

REPOSITORY
  R320 KIO Extras

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

To: sitter, ngraham, cfeck, #frameworks, #dolphin
Cc: mmustac, meven, hallas, anthonyfieroni, asturmlechner, 
kde-frameworks-devel, kfm-devel, nikolaik, pberestov, iasensio, fprice, 
LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, feverfew, michaelh, 
spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, 
mikesomov

D28513: smb: use prettyname.kio-discovery-wsd for hostname of wsdiscoveries

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> kossebau wrote in kio_smb_browse.cpp:239
> `QString::remove()` operates on the object itself, no need to assign back to 
> host.
> 
> Besides, why not use `QString::chop(wsdSuffix.size())` ?

Mh. Good point indeed, I'll move to chop. Thanks!

REPOSITORY
  R320 KIO Extras

BRANCH
  smb-lazy-resolve

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

To: sitter, ngraham, meven
Cc: kossebau, kde-frameworks-devel, kfm-devel, nikolaik, pberestov, iasensio, 
fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D28513: smb: use prettyname.kio-discovery-wsd for hostname of wsdiscoveries

[Harald Sitter]

sitter updated this revision to Diff 79220.
sitter added a comment.


  reshuffle: instead of using .local directly use a fake .kio-disocvery-wsd 
suffix. look for that at listing time and redirect to name.local or name as 
appropriate
  
  this prevents dnssd results form incorrectly getting run through the query 
branch AND has a 100% fallback chance for the llmnr/netbios name if the dnssd 
name isn't resolvable for wsdiscoveries

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28513?vs=79132=79220

BRANCH
  smb-lazy-resolve

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

AFFECTED FILES
  smb/kio_smb_browse.cpp
  smb/wsdiscoverer.cpp

To: sitter, ngraham, meven
Cc: kde-frameworks-devel, kfm-devel, nikolaik, pberestov, iasensio, fprice, 
LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D28513: smb: use prettyname.kio-discovery-wsd for hostname of wsdiscoveries

[Harald Sitter]

sitter retitled this revision from "smb: use prettyname.local for hostname of 
wsdiscoveries" to "smb: use prettyname.kio-discovery-wsd for hostname of 
wsdiscoveries".
sitter edited the summary of this revision.

REPOSITORY
  R320 KIO Extras

BRANCH
  smb-lazy-resolve

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

To: sitter, ngraham, meven
Cc: kde-frameworks-devel, kfm-devel, nikolaik, pberestov, iasensio, fprice, 
LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D28513: smb: use prettyname.local for hostname of wsdiscoveries

[Harald Sitter]

sitter added a comment.


  Stable was the plan, yes.
  
  I've thought of some complications with this approach though. Actually a 
combination of two
  
  1. the .local match also applies to dnssd
  2. all linux VMs I've checked wouldn't be able to resolve netbios names 
natively as the relevant modules are simply not loaded by default. meaning the 
netbios fallback will never be hit because 'foo' cannot ever be resolved (well, 
unless the user manually enables nmbd or lllmnr resolution)
  
  to mitigate 2 we could just always fall back to netbios regardless of its 
resolvability, but that has the problem that we can then discover DNSSD 
services on bar.local have that fall back to bar and somewhat unexpected report 
that 'bar' was not resolvable (when in fact the intended and wanted resolution 
is bar.local for the DNSSD scenario). I'm not sure it's a huge problem, but it 
certainly irks me a bit because very practically libsmbc's resolution may be 
way more flexible than the regular libc/nss resolution.
  
  OTOH fixing both would require yet more URL hackery to carry more context 
from the discovery into the list call. by either making the hostname 
`foo.kio-discovery-wsd` instead of `foo.local` or putting the discovery method 
into a query `smb://foo?kio-discovery=wsd`. I find both a bit meh. but then 
dnssd can be entirely unaffected by any of this

REPOSITORY
  R320 KIO Extras

BRANCH
  smb-lazy-resolve

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

To: sitter, ngraham, meven
Cc: kde-frameworks-devel, kfm-devel, nikolaik, pberestov, iasensio, fprice, 
LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D28513: smb: use prettyname.local for hostname of wsdiscoveries

[Harald Sitter]

sitter added a comment.


  Competes with D27902 
  While this diff runs the risk of not being able to resolve 100% of the time 
(sans ip address fallback in listDir) it has the huge advantage that discovery 
isn't getting super slow when multiple wsd hosts are found and consequently 
need costly host resolution performed. So, I like this diff mch better.

REPOSITORY
  R320 KIO Extras

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

To: sitter, ngraham, meven
Cc: kde-frameworks-devel, kfm-devel, nikolaik, pberestov, iasensio, fprice, 
LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D28513: smb: use prettyname.local for hostname of wsdiscoveries

[Harald Sitter]

sitter created this revision.
sitter added reviewers: ngraham, meven.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.
sitter requested review of this revision.

REVISION SUMMARY
  previously we simply used the ip address. this is fairly awkward though.
  instead try to deduce a resolvable host name from the pretty name.
  
  at discovery time we simply assume prettyName.local is the hostname.
  
  at listDir time we then attempt to resolve the name and if that fails
  strip the .local to get the presumed LLMNR/netbios name. this means
  the (first) listDir may be slower while we try to find a working hostname
  but discovery is still as fast as possible.

TEST PLAN
  wsdd on linux server resolves as expected, wsd on win10 also resolves as 
expected

REPOSITORY
  R320 KIO Extras

BRANCH
  smb-lazy-resolve

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

AFFECTED FILES
  smb/kio_smb_browse.cpp
  smb/wsdiscoverer.cpp

To: sitter, ngraham, meven
Cc: kde-frameworks-devel, kfm-devel, nikolaik, pberestov, iasensio, fprice, 
LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> bruns wrote in fstabhandling.cpp:131
> I got the impression you liked being lax. Guess only when the shoe is on the 
> other foot.

I do like being lax! I literally gave you a ship it despite your comment being 
literally wrong.
Picking on useless crap such as whether a comment is technically entirely 
correct or not doesn't give anything of value to anyone except the person 
picking, who is really just getting an air of superiority I'm sure. The rest of 
the world meanwhile gets their time wasted. You seem to not appreciate that, 
and I'll clearly not make you see it either. One of many great failures of 
mine, right next to wanting things get done.

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> bruns wrote in fstabhandling.cpp:131
> You let the old comment pass without any further remarks, but now you start 
> nitpicking?
> 
> From the acessibility viewpoint of the mount, it is unique.

The previous comment was factually correct, it never claimed the same source on 
the same mount would be differentiated. Your comment on the other hand does and 
it's wrong. It's doesn't say it's unique for the purposes of presentation or 
anything. It says mountpoints are unique. But they are not.
I got the impression you liked nitpicking. Guess only when the shoe is on the 
other foot.

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Harald Sitter]

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

REPOSITORY
  R245 Solid

BRANCH
  fix_comment

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> bruns wrote in fstabhandling.cpp:131
> The first mount is no longer visible, it is shadowed by the first one. You 
> can not read any files from it or write to it. You can not  unmount it.

The comment is still wrong.

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> bruns wrote in fstabhandling.cpp:131
> Of course they are - you can just mount one fs at a path at any time.

λ ajax ~ → sudo mount -t cifs //bear.local/foo /mnt -o user=me  
  λ ajax ~ → sudo mount -t cifs //bear.local/foo /mnt -o user=me
  λ ajax ~ → sudo mount --bind /tmp /mnt 
  λ ajax ~ → sudo mount --bind /tmp /mnt
  λ ajax ~ → mount|grep /mnt
  //bear.local/foo on /mnt type cifs 
(rw,relatime,vers=3.1.1,cache=strict,username=me,uid=0,noforceuid,gid=0,noforcegid,addr=fd00::::8c8d:02ff:fec7:d6a4,file_mode=0755,dir_mode=0755,soft,nounix,serverino,mapposix,rsize=4194304,wsize=4194304,bsize=1048576,echo_interval=60,actimeo=1)
  //bear.local/foo on /mnt type cifs 
(rw,relatime,vers=3.1.1,cache=strict,username=me,uid=0,noforceuid,gid=0,noforcegid,addr=2a03:c100:f100:4f00:8c8d:02ff:fec7:d6a4,file_mode=0755,dir_mode=0755,soft,nounix,serverino,mapposix,rsize=4194304,wsize=4194304,bsize=1048576,echo_interval=60,actimeo=1,user=me)
  /dev/nvme1n1p1 on /mnt type btrfs 
(rw,relatime,ssd,space_cache,subvolid=1082,subvol=/@/tmp)
  /dev/nvme1n1p1 on /mnt type btrfs 
(rw,relatime,ssd,space_cache,subvolid=1082,subvol=/@/tmp)

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28488: [Fstab] Ensure uniqueness for all filesystem types

[Harald Sitter]

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

INLINE COMMENTS

> fstabhandling.cpp:131
> +// for different users. Make sure it is unique by appending the
> +// mountpoint (which is unique).
> +return source + QLatin1Char(':') + mountpoint;

mountpoints are not unique

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, meven, broulik, sitter
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28476: Samba: Ensure to differenciate mounts sharing the same source

[Harald Sitter]

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


  You know, I may have noticed this when working on kinfocenter and then 
quickly forgotten about it again  
  Good stuff!

REPOSITORY
  R245 Solid

BRANCH
  master

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

To: meven, #frameworks, sitter
Cc: ahmadsamir, anthonyfieroni, sitter, kde-frameworks-devel, LeGast00n, 
cblack, GB_2, michaelh, ngraham, bruns