Re: Merge tags in master branch?

2020-11-23 Thread 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


D28745: Skip caching thumbnails on encrypted filesystems

2020-10-09 Thread 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

2020-10-08 Thread 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


D28855: put minimumkeepsize actually in the netpref KCM

2020-09-05 Thread 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: Extend metainfo.yaml files with License Information

2020-08-18 Thread 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

2020-08-17 Thread 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

2020-08-10 Thread 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

2020-08-06 Thread 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: Policy on forward declarations for things from external libraries

2020-08-03 Thread 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

2020-07-20 Thread 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

2020-07-15 Thread 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


D7563: Add privilegeExecution field to file protocol description

2020-06-05 Thread 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

2020-06-02 Thread 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

2020-05-26 Thread 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

2020-05-20 Thread 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

2020-05-19 Thread 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

2020-05-15 Thread 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

2020-05-14 Thread 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

2020-05-14 Thread 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

2020-05-14 Thread 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

2020-05-13 Thread 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

2020-05-13 Thread 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

2020-05-13 Thread 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

2020-05-12 Thread 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

2020-05-11 Thread 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

2020-05-06 Thread 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

2020-05-05 Thread 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

2020-05-04 Thread 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

2020-04-24 Thread 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

2020-04-24 Thread 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

2020-04-24 Thread 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

2020-04-21 Thread 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

2020-04-20 Thread 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

2020-04-19 Thread 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

2020-04-17 Thread 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

2020-04-15 Thread 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

2020-04-15 Thread 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

2020-04-15 Thread 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

2020-04-15 Thread 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

2020-04-15 Thread 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

2020-04-08 Thread 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

2020-04-08 Thread 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

2020-04-08 Thread 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

2020-04-06 Thread 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

2020-04-06 Thread 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

2020-04-06 Thread 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

2020-04-06 Thread 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

2020-04-03 Thread 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

2020-04-03 Thread 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

2020-04-03 Thread 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

2020-04-03 Thread 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

2020-04-03 Thread 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

2020-04-03 Thread 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

2020-04-03 Thread 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

2020-04-02 Thread 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

2020-04-02 Thread 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

2020-04-02 Thread 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

2020-04-02 Thread 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

2020-04-02 Thread 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

2020-04-02 Thread 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

2020-04-02 Thread 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

2020-04-02 Thread 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

2020-04-01 Thread 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


D28463: do not install testengine

2020-03-31 Thread Harald Sitter
sitter created this revision.
sitter added a reviewer: mart.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
sitter requested review of this revision.

REVISION SUMMARY
  by default users have to opt out of BUILD_TESTING meaning everyone would
  by default build and get testengine installed even though it serves no
  purpose in production. this also includes distros as I've noticed :O
  
  do not install the engine, same as testplugin isn't getting installed.
  
  (I am actually thinking throwing it away as a whole may make sense; it
   serves no real purpose over any other engine)

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

AFFECTED FILES
  tests/testengine/CMakeLists.txt

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


D28129: Read the new message string after rather than before

2020-03-27 Thread Harald Sitter
sitter accepted this revision.
sitter added a comment.


  Mind the comment about the +2 please.
  
  Other than that looks reasonable.

INLINE COMMENTS

> backtraceparsergdb.cpp:215
> +case BacktraceLine::Info:
> +d->m_infoLines << line.toString().mid(KCRASH_INFO_MESSAGE.size() + 
> 2);
> +break;

Please add a comment on what that +2 is, or better yet give it a var so it has 
an explicit name in code.

REPOSITORY
  R871 DrKonqi

BRANCH
  arcpatch-D28129

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

To: apol, #frameworks, broulik, sitter
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart


D28161: [kio-extras] Port some QRegExp usage to QRegularExpression

2020-03-26 Thread Harald Sitter
sitter added a comment.


  In D28161#634951 , @ahmadsamir 
wrote:
  
  > > Please don't touch kdsoap-ws-discovery-client though. It's not maintained 
here -> https://gitlab.com/caspermeijn/kdsoap-ws-discovery-client
  >
  > That's confusing; but OK, I'll revert that part of the diff.
  
  
  Yeah, it's not ideal but the library is not API-stable so we need a fixed 
reference point. Do please make a merge request for it though, Casper is very 
responsive to MRs.

REPOSITORY
  R320 KIO Extras

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

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


D28161: [kio-extras] Port some QRegExp usage to QRegularExpression

2020-03-26 Thread Harald Sitter
sitter added a comment.


  Looks good to me, not that this means much with regular expressions. The lack 
of tests on this stuff makes me sad :((
  
  Please don't touch kdsoap-ws-discovery-client though. It's not maintained 
here -> https://gitlab.com/caspermeijn/kdsoap-ws-discovery-client

REPOSITORY
  R320 KIO Extras

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

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


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

2020-03-25 Thread Harald Sitter
sitter added a comment.


  I've actually had a better idea! Well kinda and it may be trickier to do.
  
  We could move the resolution dance to actual resolution time so it stops 
having impact on discovery.
  i.e. we just always use `"smb://" + m_prettyName + ".local"` as UDS_URL for 
discovery. Should the user then attempt to enter that directory we'll get a 
listDir call and we call smbc_opendir(), if that fails we simply redirect to 
the url without .local, if that fails we give up. This is in fact more in line 
with how windows does it anyway, it too will only resolve when you try to list 
a server, not when it gets discovered.
  Obvious caveat is that either prettyname or prettyname.local must be 
resolvable as we cannot fall back to the IP anymore. But again, that's more in 
line with windows.
  
  Thoughts?

REPOSITORY
  R320 KIO Extras

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

To: sitter, ngraham, dfaure
Cc: meven, 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


D27873: sftp: do not show creation time when we do not know it

2020-03-25 Thread Harald Sitter
This revision was automatically updated to reflect the committed changes.
Closed by commit R320:b13449a13b00: sftp: do not show creation time when we do 
not know it (authored by sitter).

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27873?vs=78352=78444

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

AFFECTED FILES
  sftp/kio_sftp.cpp

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


D27872: sftp: fix partial transfer resuming when copying to local

2020-03-25 Thread Harald Sitter
This revision was not accepted when it landed; it landed in state "Needs 
Revision".
This revision was automatically updated to reflect the committed changes.
Closed by commit R320:8a04e1009130: sftp: fix partial transfer resuming when 
copying to local (authored by sitter).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D27872?vs=77076=78445#toc

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27872?vs=77076=78445

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

AFFECTED FILES
  sftp/kio_sftp.cpp

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


D27873: sftp: do not show creation time when we do not know it

2020-03-24 Thread Harald Sitter
sitter updated this revision to Diff 78352.
sitter added a comment.


  .

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27873?vs=77080=78352

BRANCH
  sftp-ctime

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

AFFECTED FILES
  sftp/kio_sftp.cpp

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


D27504: smb faster copy to local

2020-03-23 Thread Harald Sitter
sitter added a comment.


  Any opinions on landing this for 20.04 still? It is technically a bugfix. It 
is also practically a whole lot of risky code, making me rather uneasy about 
putting it in past beta.

REPOSITORY
  R320 KIO Extras

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

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


D28129: Read the new message string after rather than before

2020-03-23 Thread Harald Sitter
sitter added a comment.


  Mh. Not quite what I had in mind but I suppose it makes sense this way. 
  I think we need a test case for the highlighter though :| It totally blows up 
in my face when I trace a running dolphin because toskip isn't quite right.

INLINE COMMENTS

> backtracegenerator.cpp:95
>  
> +auto preamble = new QTemporaryFile;
> +preamble->open();

This is leaking the file, is it not? It never deletes this object.

> gdbhighlighter.cpp:59
>  int lineNr = currentBlock().firstLineNumber();
> +int toskip = 1 + m_infoLinesCount; // 1st line contains 'Application: 
> ...'
>  while ( cur < text.length() ) {

lineNr is initialized to currentBlock().firstLineNumber() that is not 
necessarily 0, so toskip needs to be 
`lineNr + 1 + infocount` otherwise the skipping doesn't work as expected.

Should also be camel toSkip.

> gdbhighlighter.cpp:65
>  }
> -if (lineNr == 0) {
> -// line that contains 'Application: ...'
> +if (lineNr <= toskip) {
>  ++lineNr;

Isn't this off-by-one versus the original code?
Say we have no infolines we'd then skip
[0] and [1] when previously we'd only skip [0].

> gdbhighlighter.cpp:76
> +// toskip since we skip the first line and the info lines
> +QMap< int, BacktraceLine >::iterator it = lines.lowerBound(lineNr - 
> toskip);
>  Q_ASSERT(it != lines.end());

The assert below fails when I trace a running dolphin, I am not super sure why 
but I am guessing it's because the toskip init being bugged vis a vis the 
lineNr being an offset.

REPOSITORY
  R871 DrKonqi

BRANCH
  master

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

To: apol, #frameworks, broulik, sitter
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart


D28188: breeze-icons autotests: skip symlinks in the builddir

2020-03-22 Thread Harald Sitter
sitter accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R266 Breeze Icons

BRANCH
  master

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

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


D27927: Use more flexible shebang

2020-03-19 Thread Harald Sitter
This revision was automatically updated to reflect the committed changes.
Closed by commit R266:485e7be3b7ef: Use more flexible shebang (authored by 
tcberner, committed by sitter).

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27927?vs=77198=78011

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

AFFECTED FILES
  generate-24px-versions.sh

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


D28129: Read the new message string after rather than before

2020-03-19 Thread Harald Sitter
sitter added a comment.


  Quick recap from what we talked about on telegram: putting the print after 
the bt is most definitely going to throw off the backtrace parsing logic, so 
doing it this way would require extensive changes there, which is a dangerous 
place to make extensive changes.
  Or we could define a simple function to ignore errors 
https://stackoverflow.com/questions/17923865/gdb-stops-in-a-command-file-if-there-is-an-error-how-to-continue-despite-the-er
 but that's also a bit faffy.
  Another completely standalone approach would be to change BacktraceGenerator 
(I think?) to invoke the print in a completely independent gdb invocation i.e. 
separate the print call from the regular batchcommands and have drkonqi 
assemble it back into the final report. That way the actual tracing batch 
command couldn't fail on the print.

REPOSITORY
  R871 DrKonqi

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

To: apol, #frameworks, broulik, sitter
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart


D18878: Force NT1 protocol when listing smb:// network [work in progress]

2020-03-19 Thread Harald Sitter
sitter removed a reviewer: sitter.

REPOSITORY
  R320 KIO Extras

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

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


D25984: Load translations

2020-03-19 Thread Harald Sitter
sitter removed a reviewer: sitter.

REPOSITORY
  R169 Kirigami

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

To: broulik, #kirigami, #frameworks, kossebau, aacid
Cc: mart, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, 
dkardarakos, ngraham, apol, ahiemstra


D27871: sftp: fix seekPos + file resuming when part file is of size 11

2020-03-19 Thread Harald Sitter
This revision was automatically updated to reflect the committed changes.
Closed by commit R320:d8cf85ec2f0f: sftp: fix seekPos + file resuming when part 
file is of size 11 (authored by sitter).

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27871?vs=77036=77991

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

AFFECTED FILES
  sftp/kio_sftp.cpp

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


D28078: guard against duplicated service discoveries

2020-03-19 Thread Harald Sitter
This revision was automatically updated to reflect the committed changes.
Closed by commit R320:ba96d3b93a96: smb: guard against duplicated service 
discoveries (authored by sitter).

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28078?vs=77737=77990

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

AFFECTED FILES
  smb/discovery.h
  smb/dnssddiscoverer.cpp
  smb/dnssddiscoverer.h
  smb/kio_smb_browse.cpp
  smb/wsdiscoverer.cpp
  smb/wsdiscoverer.h

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


D28123: move setErrorMessage definition out of the linux ifdef

2020-03-18 Thread Harald Sitter
This revision was automatically updated to reflect the committed changes.
Closed by commit R285:91129f291efd: move setErrorMessage definition out of the 
linux ifdef (authored by sitter).

REPOSITORY
  R285 KCrash

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28123?vs=77910=77913

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

AFFECTED FILES
  src/kcrash.cpp

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


D28123: move setErrorMessage definition out of the linux ifdef

2020-03-18 Thread Harald Sitter
sitter created this revision.
sitter added a reviewer: apol.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
sitter requested review of this revision.

REVISION SUMMARY
  rather obvious in retrospect. the definition is OS-independent,
  the declaration naturally should be as well.
  this fixes builds of drkonqi on all systems that aren't
  linux.
  
  this file has way too many, way too long ifdefs...

TEST PLAN
  drkonqi builds on windows

REPOSITORY
  R285 KCrash

BRANCH
  master

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

AFFECTED FILES
  src/kcrash.cpp

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


D28121: Use libcanberra as primary means of previewing the sound

2020-03-18 Thread Harald Sitter
sitter accepted this revision.
sitter added a comment.
This revision is now accepted and ready to land.


  Your `if(` style is now inconsistent with the `endif(`.
  Code LGTM

REPOSITORY
  R305 KNotifyConfig

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

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


D28095: Bump required version of KF5 to 5.69.0

2020-03-17 Thread Harald Sitter
sitter accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R871 DrKonqi

BRANCH
  l-kcrash (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, apol, sitter
Cc: kde-frameworks-devel, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart


D28078: guard against duplicated service discoveries

2020-03-16 Thread Harald Sitter
sitter added a comment.


  FTR: this could technically still dupe with smbc native NT1 listing, except 
smbc would list workgroups while we list services, so this should generally not 
be duplicative information.

REPOSITORY
  R320 KIO Extras

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

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


D28078: guard against duplicated service discoveries

2020-03-16 Thread 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
  if both dnssd and wsdiscovery have the same server on offer we'd create
  duplicated entries. instead keep track of the discovered names and only
  use the first discovery. notably happens when using wsdd on linux where
  samba would also register itself on dnssd.
  
  for ease of access and to not have to roundtrip through udsentry, discovery
  objects now have a udsName function that is meant to be the exact name
  used for the udsentry.

TEST PLAN
  - have samba
  - run wsdd.py from https://github.com/christgau/wsdd
  - server only shows up once

REPOSITORY
  R320 KIO Extras

BRANCH
  smb-no-dupes

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

AFFECTED FILES
  smb/discovery.h
  smb/dnssddiscoverer.cpp
  smb/dnssddiscoverer.h
  smb/kio_smb_browse.cpp
  smb/wsdiscoverer.cpp
  smb/wsdiscoverer.h

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


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

2020-03-16 Thread Harald Sitter
sitter added a comment.


  Yep. I'm 100% certain of this. The library in fact has no API that returns a 
complete URL or anything near a complete URL. It's using dirent-inspired API to 
let us iterate/stat paths and only ever returns paths relative to whatever 
input it got, from those paths we then compose the actual output URLs again.

REPOSITORY
  R320 KIO Extras

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

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


D27520: add readdirplus2 support for samba>=4.12

2020-03-16 Thread Harald Sitter
This revision was automatically updated to reflect the committed changes.
Closed by commit R320:ec40cd6ef3f4: add readdirplus2 support for samba=4.12 
(authored by sitter).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D27520?vs=76048=77717#toc

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27520?vs=76048=77717

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

AFFECTED FILES
  smb/CMakeLists.txt
  smb/config-smb.h.cmake
  smb/kio_smb.h
  smb/kio_smb_browse.cpp

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


D27872: sftp: fix partial transfer resuming when copying to local

2020-03-16 Thread Harald Sitter
sitter added a comment.


  ping

REPOSITORY
  R320 KIO Extras

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

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


D27873: sftp: do not show creation time when we do not know it

2020-03-16 Thread Harald Sitter
sitter added a comment.


  ping

REPOSITORY
  R320 KIO Extras

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

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


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

2020-03-16 Thread Harald Sitter
sitter added a comment.


  ping

REPOSITORY
  R320 KIO Extras

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

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


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

2020-03-16 Thread Harald Sitter
sitter added a comment.


  In D27804#621988 , @sitter wrote:
  
  > In D27804#621970 , @thiago wrote:
  >
  > > Still want to see that round-trip.
  >
  >
  > But why? Converting an smbcUrl to a QUrl would literally be useless code.
  
  
  @thiago ^

REPOSITORY
  R320 KIO Extras

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

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


D28014: smb: do not double finish

2020-03-16 Thread Harald Sitter
This revision was automatically updated to reflect the committed changes.
Closed by commit R320:8252861157a2: smb: do not double finish (authored by 
sitter).

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28014?vs=77502=77714

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

AFFECTED FILES
  smb/kio_smb_mount.cpp

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


D28061: Reduce unneeded dependencies

2020-03-16 Thread Harald Sitter
sitter accepted this revision.

REPOSITORY
  R871 DrKonqi

BRANCH
  master

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

To: apol, #frameworks, davidedmundson, sitter
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart


D21466: Recommend rebooting after installing Samba

2020-03-13 Thread Harald Sitter
sitter edited the summary of this revision.

REPOSITORY
  R432 File Sharing (Samba) integration

BRANCH
  recommend-rebooting (branched from master)

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

To: ngraham, #vdg, #frameworks, #dolphin, apol, sitter
Cc: anthonyfieroni, sitter, bruns


D27986: Allow providing an error message from the application

2020-03-13 Thread Harald Sitter
sitter added a comment.


  Looks alright to me now. 

REPOSITORY
  R285 KCrash

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

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


D27986: Allow providing an error message from the application

2020-03-13 Thread Harald Sitter
sitter added inline comments.

INLINE COMMENTS

> kcrash.cpp:107
> +
> +static char *s_kcrashErrorMessage;
>  Q_GLOBAL_STATIC(KCrash::CoreConfig, s_coreConfig)

Still needs to be set to nullptr.

REPOSITORY
  R285 KCrash

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

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


D27986: Allow providing an error message from the application

2020-03-13 Thread Harald Sitter
sitter added inline comments.

INLINE COMMENTS

> kcrash.cpp:107
> +
> +static char* s_kcrashErrorMessage;
>  Q_GLOBAL_STATIC(KCrash::CoreConfig, s_coreConfig)

- on wrong side of space

needs defining to nullptr

> kcrash.cpp:994
> +{
> +s_kcrashErrorMessage = qstrdup(message.toUtf8().constData());
> +}

I fear that is now leaking when the function is called more than once

  if (s_kcrashErrorMessage) {
  free(s_kcrashErrorMessage);
  }

REPOSITORY
  R285 KCrash

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

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


D28014: smb: do not double finish

2020-03-12 Thread 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
  finish() must not be called twice yet previously the case branches
  would call it but then there'd also be a global finished after the switch.
  remove the cases' finished().
  
  this code is, I think, actually not used anywhere so it matters little.

TEST PLAN
  hoping for the best!

REPOSITORY
  R320 KIO Extras

BRANCH
  smb-mount-doublefinish

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

AFFECTED FILES
  smb/kio_smb_mount.cpp

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


D21466: Recommend rebooting after installing Samba

2020-03-12 Thread Harald Sitter
sitter accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R432 File Sharing (Samba) integration

BRANCH
  recommend-rebooting (branched from master)

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

To: ngraham, #vdg, #frameworks, #dolphin, apol, sitter
Cc: anthonyfieroni, sitter, bruns


  1   2   3   4   5   6   7   8   9   10   >