Re: KDEReview for kio-s3

[Elvis Angelaccio]

On 20/09/22 23:55, Albert Astals Cid wrote:

El dimarts, 20 de setembre de 2022, a les 0:17:52 (CEST), Elvis Angelaccio va
escriure:

Hi all,
I think kio-s3 [1] is now ready to go through the KDEReview process and
get a first release.

dfaure and sitter did a first code review in the original MR against
kio-extras [1]. The code hasn't changed much since then, mostly the port
to the new WorkerBase interface.

Please have a look. Thanks :)


Small enough, i didn't find anything to mention :)

Cheers,
   Albert


Thanks Albert.

I've now updated the projectpath on the metadata repo. Will soon do a 
first beta release.


Cheers,
Elvis



P.S.
You might notice that the gitlab CI is currently broken, I have already
filed a sysadmin ticket trying to sort it out.

[1]: https://invent.kde.org/network/kio-s3
[2]: https://invent.kde.org/network/kio-extras/-/merge_requests/35


Cheers,
Elvis

KDEReview for kio-s3

[Elvis Angelaccio]

Hi all,
I think kio-s3 [1] is now ready to go through the KDEReview process and 
get a first release.


dfaure and sitter did a first code review in the original MR against 
kio-extras [1]. The code hasn't changed much since then, mostly the port 
to the new WorkerBase interface.


Please have a look. Thanks :)

P.S.
You might notice that the gitlab CI is currently broken, I have already 
filed a sysadmin ticket trying to sort it out.


[1]: https://invent.kde.org/network/kio-s3
[2]: https://invent.kde.org/network/kio-extras/-/merge_requests/35


Cheers,
Elvis

D29525: Make Previews devicePixelRatio aware

[Elvis Angelaccio]

elvisangelaccio added a comment.


  Superseded by https://invent.kde.org/system/dolphin/-/merge_requests/147
  
  @meven Can you abandon this one?

REPOSITORY
  R318 Dolphin

BRANCH
  arcpatch-D29525

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

To: meven, #dolphin, #frameworks, dfaure, ngraham, elvisangelaccio
Cc: kfm-devel, badbunny, waitquietly, azyx, nikolaik, pberestov, iasensio, 
aprcela, fprice, fbampaloukas, alexde, Codezela, feverfew, meven, spoorun, 
navarromorales, firef, ngraham, andrebarros, emmanuelp, rdieter, mikesomov

Re: Please review KUserFeedback support in Dolphin

[Elvis Angelaccio]

Thanks Nicolas and Christoph for your feedback.

I'll be pushing the MR tomorrow before the 20.12 dependency freeze, 
unless there are objections.


On 29/10/20 22:05, Christoph Cullmann wrote:

On 2020-10-29 21:46, Elvis Angelaccio wrote:

Hi everyone,

I was planning to add KUserFeedback in Dolphin for the upcoming 20.12
release, but then I realized that our telemetry policy [1] says we
should go through a public review on kde-core-devel first.

So here I am, this is the MR:
https://invent.kde.org/system/dolphin/-/merge_requests/45

Please have a look.

Dependency freeze for 20.12 is on November 5th, so in one week. I'd
appreciate some feedback before that.

Thanks :)


Hi,

could we make KUserFeedback a framework then, to ensure we have some
proper released and updated foundation, if we use that in more stuff now?


+1


Cheers,
Elvis



Greetings
Christoph



Cheers,
Elvis

[1]: https://community.kde.org/Policies/Telemetry_Policy#Compliance

Please review KUserFeedback support in Dolphin

[Elvis Angelaccio]

Hi everyone,

I was planning to add KUserFeedback in Dolphin for the upcoming 20.12 
release, but then I realized that our telemetry policy[1] says we should 
go through a public review on kde-core-devel first.


So here I am, this is the MR: 
https://invent.kde.org/system/dolphin/-/merge_requests/45


Please have a look.

Dependency freeze for 20.12 is on November 5th, so in one week. I'd 
appreciate some feedback before that.


Thanks :)

Cheers, Elvis

[1]: https://community.kde.org/Policies/Telemetry_Policy#Compliance

D23694: Add support for sshfs to the fstab backend

[Elvis Angelaccio]

elvisangelaccio added a comment.


  In D23694#676619 , 
@elvisangelaccio wrote:
  
  > In D23694#676610 , @lbeltrame 
wrote:
  >
  > > @elvisangelaccio Thanks, first of all. If that's the case, do you think 
it's better to touch KIO first before landing this?
  >
  >
  > Yeah this should probably wait for the KIO patch, which I just submitted: 
https://invent.kde.org/frameworks/kio/-/merge_requests/175
  
  
  The KIO patch has been merged.
  
  +1 for merging this one as well.

REPOSITORY
  R245 Solid

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

To: lbeltrame, bruns, broulik, fvogt, #kde_connect
Cc: elvisangelaccio, albertvaka, ngraham, kde-frameworks-devel, LeGast00n, 
cblack, michaelh, bruns

D23694: Add support for sshfs to the fstab backend

[Elvis Angelaccio]

elvisangelaccio added a comment.


  In D23694#676610 , @lbeltrame 
wrote:
  
  > @elvisangelaccio Thanks, first of all. If that's the case, do you think 
it's better to touch KIO first before landing this?
  
  
  Yeah this should probably wait for the KIO patch, which I just submitted: 
https://invent.kde.org/frameworks/kio/-/merge_requests/175

REPOSITORY
  R245 Solid

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

To: lbeltrame, bruns, broulik, fvogt, #kde_connect
Cc: elvisangelaccio, albertvaka, ngraham, kde-frameworks-devel, LeGast00n, 
cblack, michaelh, bruns

D23694: Add support for sshfs to the fstab backend

[Elvis Angelaccio]

elvisangelaccio added a comment.


  In D23694#676607 , @lbeltrame 
wrote:
  
  > Would this change be needed on this side or kdeconnect side? I don't mind 
landing this, but I need formal approval by someone. (I won't have any time to 
do additional changes elsewhere).
  
  
  The change I was talking about would be in KIO (in the places model). I can 
do that.
  
  In D23694#676608 , @bruns wrote:
  
  > One possibility to hide it would be the `x-gvfs-*` options, though I am not 
sure if it is possible to pass these to fuse.
  
  
  Doesn't seem to work :(
  
sshfs -o x-gvfs-hide nuc:/data/ /home/elvis/DATA/
fuse: unknown option(s): `-o x-gvfs-hide'

sshfs -o comments=x-gvfs-hide nuc:/data/ /home/elvis/DATA/
fuse: unknown option(s): `-o comments=x-gvfs-hide'

REPOSITORY
  R245 Solid

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

To: lbeltrame, bruns, broulik, fvogt, #kde_connect
Cc: elvisangelaccio, albertvaka, ngraham, kde-frameworks-devel, LeGast00n, 
cblack, michaelh, bruns

D23694: Add support for sshfs to the fstab backend

[Elvis Angelaccio]

elvisangelaccio added a comment.


  Ping? Can we get this in?
  
  In D23694#525220 , @albertvaka 
wrote:
  
  > If it's a problem for kdeconnect mounts to appear there, how can we hide 
it? It's an sshfs mountpoint like any other, only that it is mounted 
programmatically.
  
  
  We should be able to easily hide it from KIO, because we know that the mount 
point of the place URL will be `kdeconnect@X.X.X.X`

REPOSITORY
  R245 Solid

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

To: lbeltrame, bruns, broulik, fvogt, #kde_connect
Cc: elvisangelaccio, albertvaka, ngraham, kde-frameworks-devel, LeGast00n, 
cblack, michaelh, bruns

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[Elvis Angelaccio]

On 02/10/20 19:39, Nate Graham wrote:
> Hello folks,

Hi

> I've been told that our Sysadmins have developed some tooling capable of
> checking the "Squash when merging" checkbox by default for new Merge
> Requests. This would be a downstream solution to
> https://gitlab.com/gitlab-org/gitlab/-/issues/222175.
> 
> I'd like to propose that this be done as a sane default for new Merge
> Requests. Now, personally I have gotten used to the alternative "curate
> your MR's git history" approach and have written documentation for it at
> https://community.kde.org/Infrastructure/GitLab#Curating_your_merge_request_commit_history.
> 
> 
> However, it remains a fairly advanced workflow which is challenging for
> newcomers, drive-by-developers, and people not as familiar with git. For
> these people, squash-merging makes much more sense, and when they forget
> to check that checkbox and someone merges their work, the result is tons
> of garbage commits in the git history.

People should not blindly merge a MR without checking the commit history
first:
- if the submitter pushed one or more well-written atomic commits, those
can be merged without squashing
- otherwise, whoever merges the MR should squash the history and write a
proper commit message. Or they should ask the submitter to fix its history.

> 
> Accordingly, I think squash-merging makes sense as the default value to
> avoid this. People who are comfortable with the "curated MR commit
> history" workflow will of course still be able to turn it off. IMO it
> makes more sense to ask experts to turn it off than to ask newcomers and
> novices to turn it on.

+0, I don't have a preference for this default value. As explained, it's
orthogonal to the problem of a bad commit history.

> 
> Thoughts?
> 
> Nate

Cheers,
Elvis

Re: Git merge workflow: reverse it?

[Elvis Angelaccio]

On 24/08/20 22:07, b...@valdyas.org wrote:
> On 2020-08-24 21:55, Albert Astals Cid wrote:
> 
>> I disagree for it to per project, it has to be a global policy,
>> otherwise contributing gets more complicated, since I have to remember
>> for each project if they want bugfix patches to master or to stable.
> 
> The current "global policy" isn't global already, of course. I'd never
> even heard of it. And if I had, I would never have accepted it, since
> it's bad practice. You don't merge stable branches into unstable, or the
> other way around. You keep track of your patches (more or less
> succesfully)

I like to use git for that because it's designed for this task. :)

> and cherry-pick or port them.

Or you can merge stable to master and be sure you won't forget anything.
Of course if master changed a lot you can't (easily) do that. But we
have a lot of repos that don't change very often and merging stable to
master works very well with them.

> 
> It's the maintainer's job to keep track of that, or make sure the other
> team members know what they should do.
> 
>> Or at least it has to be the same for all the release service
>> projects, because otherwise it'll break my brain when doing the
>> branching for new releases. Right now i know that we always commit to
>> stable and then merge to master, but since people are lazy i know i
>> have to run a merge from stable to master before branch for all
>> projects, if we let this decision to be per project, what do I do?
> 
> You don't do anything; if the maintainers of those projects cannot
> manage their patch workflow, well, shucks. And if you're the maintainer,
> then you need to keep track of all that.

The problem is we don't always have a maintainer. A lot of apps are
community-maintained and if we wouldn't merge stable to master before a
new release we would reintroduce fixed bugs quite often.

> 
> Boudewijn

Cheers,
Elvis

Re: Git merge workflow: reverse it?

[Elvis Angelaccio]

On 24/08/20 21:55, Albert Astals Cid wrote:
> El dilluns, 24 d’agost de 2020, a les 0:39:17 CEST, Elvis Angelaccio va 
> escriure:
>>
>> On 29/07/20 14:01, Bhushan Shah wrote:
>>> Hello everyone!
>> Hi Bhushan
>>>
>>> At plasma, we are experimenting with new workflow regarding how bugfixes
>>> are put on the stable branch [1].
>>>
>>> # Previous workflow
>>>
>>> - Current workflow is that we commit to stable branch and then merge it
>>>   upwords until master branch
>>> - i.e commit to Plasma/5.18 branch, merge 5.18 into 5.19 and then
>>>   master
>>>
>>> # Current workflow
>>>
>>> - Proposed workflow is to instead commit all changes in master, and
>>>   cherry-pick related changes in the stable branch as needed
>>> - We had been using this workflow for about 1 month now and I'd say it
>>>   is working nicely for us.
>>>
>>> # Why?
>>>
>>> We occasionally hit several issues with previous workflow,
>>>
>>> - Merge conflicts when merging changes upwords
>>> - Changes which are valid only for stable branch needs to be reverted in
>>>   master branches. So you end-up with, stable-fix, revert of stable fix
>>>   and then different fix and overall weird history.
>>> - Accidential merges from the master branch to stable branch which
>>>   needs to be force-resetted.
>>> - It's worth noting that Qt also recently changed to merge to dev,
>>>   cherry-pick backwards.
>>> - This also allows for workflows where we want to commit some bugfix in
>>>   the master branch for few days/weeks and if it works fine in general
>>>   testing then, cherry-pick it in stable branches.
>>>
>>> Proposal is to probably adapt this policy kde-wise if people feel that
>>> advantages are worth it.
>> IMHO the workflow choice should be left to each project. 
> 
> I disagree for it to per project, it has to be a global policy, otherwise 
> contributing gets more complicated, since I have to remember for each project 
> if they want bugfix patches to master or to stable.
> 
> Or at least it has to be the same for all the release service projects, 
> because otherwise it'll break my brain when doing the branching for new 
> releases. Right now i know that we always commit to stable and then merge to 
> master, but since people are lazy i know i have to run a merge from stable to 
> master before branch for all projects, if we let this decision to be per 
> project, what do I do?
> 
>> I can see why
>> you may want to use it in Plasma, but for the apps I maintain it would
>> probably double the work for no practical gain.
> 
> Why would it be double the work?

I was mainly referring to the amount of work for tests. The git part is
trivial either way indeed.

> 
> Now:
>  * Press the merge button in the gitlab UI
>  * git fetch && git checkout master && git pull --rebase && git merge 
> origin/stable/branch
> 
> After:
>  * Press the merge button in the gitlab UI
>  * git fetch && git checkout stable && git pull --rebase && git cherry-pick 
> origin/master
> 
> Seems the same amount of work to me (if there's no conflicts)

Now:
* Test the fix on stable
* Push to stable
* Merge stable to master

After:
* Test the fix on master
* Push to master
* Test the fix on stable
* Cherry-pick to stable

In the second model I need to test twice because I can't change the
stable branch without testing.

> 
> Cheers,
>   Albert

Cheers,
Elvis

Re: Git merge workflow: reverse it?

[Elvis Angelaccio]

On 29/07/20 14:01, Bhushan Shah wrote:
> Hello everyone!
Hi Bhushan
> 
> At plasma, we are experimenting with new workflow regarding how bugfixes
> are put on the stable branch [1].
> 
> # Previous workflow
> 
> - Current workflow is that we commit to stable branch and then merge it
>   upwords until master branch
> - i.e commit to Plasma/5.18 branch, merge 5.18 into 5.19 and then
>   master
> 
> # Current workflow
> 
> - Proposed workflow is to instead commit all changes in master, and
>   cherry-pick related changes in the stable branch as needed
> - We had been using this workflow for about 1 month now and I'd say it
>   is working nicely for us.
> 
> # Why?
> 
> We occasionally hit several issues with previous workflow,
> 
> - Merge conflicts when merging changes upwords
> - Changes which are valid only for stable branch needs to be reverted in
>   master branches. So you end-up with, stable-fix, revert of stable fix
>   and then different fix and overall weird history.
> - Accidential merges from the master branch to stable branch which
>   needs to be force-resetted.
> - It's worth noting that Qt also recently changed to merge to dev,
>   cherry-pick backwards.
> - This also allows for workflows where we want to commit some bugfix in
>   the master branch for few days/weeks and if it works fine in general
>   testing then, cherry-pick it in stable branches.
> 
> Proposal is to probably adapt this policy kde-wise if people feel that
> advantages are worth it.
IMHO the workflow choice should be left to each project. I can see why
you may want to use it in Plasma, but for the apps I maintain it would
probably double the work for no practical gain.

I'd be ok with a global policy "if unsure always send a MR against
master" (newcomers already do that anyway), but then it should be up to
the maintainer to choose how to handle the MR.

> 
> Thanks
> 
> [1] https://mail.kde.org/pipermail/plasma-devel/2020-June/117887.html
> 

Cheers,
Elvis

Re: KDEREVIEW: Proposing utilities/markdownpart to become community/release-service-managed

[Elvis Angelaccio]

On 15/08/20 19:20, Friedrich W. H. Kossebau wrote:
> Hi,
> 
> what is markdownpart you ask? A KParts plugin allowing to view markdown 
> documents rendered e.g. in Kate's preview plugin, Ark, Krusader or Konqueror, 
> being mainly a wrapper around QTextDocument::setMarkdown & QTextBrowser.
> See here it being used with Kate's preview plugin:
> https://cdn.kde.org/screenshots/markdownpart/markdownpart.png
> Note: for now Qt 5.15-only, 5.14 possible but untested.
> 
> I would like to propose markdownpart for a move into community maintenance 
> mode and becoming part of release service 

Hi Friedrich,

why not merge this plugin into kate instead?

> 
> Cheers
> Friedrich
> 
> 

Cheers,
Elvis

Re: KDEREVIEW: Proposing utilities/markdownpart to become community/release-service-managed

[Elvis Angelaccio]

On 17/08/20 23:04, Friedrich W. H. Kossebau wrote:
> Hi Elvis,
> 
> Am Montag, 17. August 2020, 22:43:37 CEST schrieb Elvis Angelaccio:
>> On 15/08/20 19:20, Friedrich W. H. Kossebau wrote:
>>> Hi,
>>>
>>> what is markdownpart you ask? A KParts plugin allowing to view markdown
>>> documents rendered e.g. in Kate's preview plugin, Ark, Krusader or
>>> Konqueror, being mainly a wrapper around QTextDocument::setMarkdown &
>>> QTextBrowser. See here it being used with Kate's preview plugin:
>>> https://cdn.kde.org/screenshots/markdownpart/markdownpart.png
>>> Note: for now Qt 5.15-only, 5.14 possible but untested.
>>>
>>> I would like to propose markdownpart for a move into community maintenance
>>> mode and becoming part of release service
>>
>> Hi Friedrich,
>>
>> why not merge this plugin into kate instead?
> 
> Let me answer with another question:
> why with Kate and not with Ark or with Krusader or with Konqueror or any 
> other 
> potential KParts plugin user where Markdown viewing often is useful?
> 
> Bundling with Kate would be a rather random choice IMHO. And
> a) make it challenging for packagers to split out an independent packager for 
> the markdown kpart with all the files belonging to it
> b) make it harder for anyone interested in hacking on it, as they would have 
> to also care about the whole Kate repo and its complete build system, when 
> they just want to improve the markdown viewer e.g. for Ark.
> 
> But not my call, after all I offer this to KDE community for adoption, sou 
> your choice.

My bad, I didn't realize this was a KPart plugin. The Kate screenshot
you linked got me confused :p

Then yes, a stand-alone package makes sense indeed.

> 
> Cheers
> Friedrich
> 
> 

Cheers,
Elvis

CMake source files without license

[Elvis Angelaccio]

Hi,

I was wondering, is there a reason why our CMakeLists.txt files often do
not contain a license?

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

Thanks.

Cheers,
Elvis

D21695: Add FindTaglib.cmake

[Elvis Angelaccio]

elvisangelaccio commandeered this revision.
elvisangelaccio added a reviewer: heikobecker.
elvisangelaccio added a comment.


  In D21695#675056 , 
@elvisangelaccio wrote:
  
  > New attempt at 
https://invent.kde.org/frameworks/extra-cmake-modules/-/merge_requests/6
  
  
  Merged, closing.

REPOSITORY
  R240 Extra CMake Modules

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

To: elvisangelaccio, kde-buildsystem, kde-frameworks-devel, dfaure, heikobecker
Cc: asturmlechner, ngraham, elvisangelaccio, cgiboudeaux, dfaure, LeGast00n, 
cblack, bencreasy, michaelh, bruns

D21695: Add FindTaglib.cmake

[Elvis Angelaccio]

elvisangelaccio abandoned this revision.

REPOSITORY
  R240 Extra CMake Modules

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

To: elvisangelaccio, kde-buildsystem, kde-frameworks-devel, dfaure, heikobecker
Cc: asturmlechner, ngraham, elvisangelaccio, cgiboudeaux, dfaure, LeGast00n, 
cblack, bencreasy, michaelh, bruns

D21695: Add FindTaglib.cmake

[Elvis Angelaccio]

elvisangelaccio added a comment.


  New attempt at 
https://invent.kde.org/frameworks/extra-cmake-modules/-/merge_requests/6

REPOSITORY
  R240 Extra CMake Modules

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

To: heikobecker, kde-buildsystem, kde-frameworks-devel, dfaure
Cc: asturmlechner, ngraham, elvisangelaccio, cgiboudeaux, dfaure, LeGast00n, 
cblack, bencreasy, michaelh, bruns

D29525: Make Previews devicePixelRatio aware

[Elvis Angelaccio]

elvisangelaccio accepted this revision.

REPOSITORY
  R318 Dolphin

BRANCH
  arcpatch-D29525

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

To: meven, #dolphin, #frameworks, dfaure, ngraham, elvisangelaccio
Cc: kfm-devel, waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, 
fprice, fbampaloukas, alexde, Codezela, feverfew, meven, spoorun, 
navarromorales, firef, ngraham, andrebarros, emmanuelp, rdieter, mikesomov

D29397: KPreviewJob : Support for DeviceRatioPixel

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> previewjob.cpp:587
> +if 
> (thumb.textKeys().contains(QStringLiteral("Thumb::DevicePixelRatio"))) {
> +qreal dpr = 
> thumb.text(QStringLiteral("Thumb::DevicePixelRatio")).toDouble();
> +thumb.setDevicePixelRatio(dpr);

Missing `const`

> previewjob.h:192
> +/**
> + * Request preview to use the device pixel ratio @p.
> + * The returned thumbnail may not respect the device pixel ratio 
> returned.

Did you mean `@p dpr`?

(unless doxygen is smart enough to figure out the name of the parameter?)

> previewjob.h:268
> + */
> +static void setDefaultDevicePixelRatio(qreal devicePixerRatio);
>  };

Typo: `devicePixelRatio`.

REPOSITORY
  R241 KIO

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

To: meven, dfaure, broulik, #frameworks, ngraham
Cc: elvisangelaccio, kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, 
cblack, michaelh, ngraham, bruns

Re: Transition to Gitlab Complete

[Elvis Angelaccio]

On 17/05/20 12:47, Ben Cooksley wrote:
> Good morning all,

Hi Ben


> 
> -- Migration Helpers --
> 
> To assist with the migration to Gitlab, Sysadmin earlier mentioned a
> series of helper tools which could be used to clone repositories
> conveniently from Gitlab as well as migrate existing clones.
> 
> To set these up, you will need a clone of 'sysadmin/repo-metadata' on
> your local system, along with Python3 and PyYAML (packaged as
> python3-yaml on most distributions) installed on your system.
> Once you have these, add the folder 'git-helpers' in
> 'sysadmin/repo-metadata' to $PATH and you should be good to go.
> 
> Please note that 'git kpull' at the moment is only able to perform the
> initial migration from anongit.kde.org/git.kde.org to invent.kde.org
> urls.
> Additional patches to improve this are welcome.

I tried to 'git kpull' from a bunch of clones I have but I always get
this error:

$ git kpull
/home/elvis/dev/kde/sysadmin-repo-metadata/git-helpers/git-kpull:36:
YAMLLoadWarning: calling yaml.load() without Loader=... is deprecated,
as the default Loader is unsafe. Please read https://msg.pyyaml.org/load
for full details.
  metadata = yaml.load( metadataFile )
Sorry, we were unable to determine the name of the repository as it is
not configured to push to the old server (git.kde.org)

and also this one:

$ git remote show origin
fatal: unable to update url base from redirection:
  asked for: https://invent.kde.org/kio/info/refs?service=git-upload-pack
   redirect: https://invent.kde.org/users/sign_in


Am I missing something?

> 
> Thanks,
> Ben Cooksley
> KDE Sysadmin
> 

Thanks,
Elvis

D29397: KPreviewJob : Support for DeviceRatioPixel

[Elvis Angelaccio]

elvisangelaccio added a comment.


  Would it make sense to initialize `devicePixelRatio` to 
`QGuiApplication::devicePixelRatio()` and add an API to change it (if desired) ?
  
  This way we wouldn't need to call a static method in the `main` of every 
client app (i.e. D29525  wouldn't be 
needed).

REPOSITORY
  R241 KIO

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

To: meven, dfaure, broulik, #frameworks, ngraham
Cc: elvisangelaccio, kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, 
cblack, michaelh, ngraham, bruns

D29197: filenamesearch: Implement stat to display metainfo

[Elvis Angelaccio]

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

REPOSITORY
  R320 KIO Extras

BRANCH
  master

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

To: meven, #dolphin, #frameworks, ngraham, elvisangelaccio
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

D29198: filenamesearch:/ define a title for the query

[Elvis Angelaccio]

elvisangelaccio accepted this revision.
elvisangelaccio added a subscriber: iasensio.
elvisangelaccio added a comment.
This revision is now accepted and ready to land.


  LGTM but I'd like input from @iasensio too.

INLINE COMMENTS

> dolphinsearchbox.h:164
>  private:
> +QString getQueryTitle(const QString& text) const;
> +

Please drop the `get` prefix.

REPOSITORY
  R318 Dolphin

BRANCH
  arcpatch-D29198

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

To: meven, ngraham, elvisangelaccio, #dolphin, #frameworks
Cc: iasensio, kfm-devel, azyx, nikolaik, pberestov, aprcela, fprice, 
fbampaloukas, alexde, Codezela, feverfew, meven, spoorun, navarromorales, 
firef, ngraham, andrebarros, emmanuelp, rdieter, mikesomov

D7563: Add privilegeExecution field to file protocol description

[Elvis Angelaccio]

elvisangelaccio added a comment.


  In D7563#650117 , @ngraham wrote:
  
  > [insert I-have-no-idea-what-I'm-doing dog meme here]
  >
  > When trying to create items in root-owned locations, I'm getting an errors 
saying "The process for the file protocol died unexpectedly." or else Dolphin 
simply crashes with a totally unhelpful backtrace.
  
  
  If it's still happening, please have a look at 
https://community.kde.org/Guidelines_and_HOWTOs/Debugging/Debugging_IOSlaves

REPOSITORY
  R241 KIO

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

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

D28802: Add standard shortcut for "Show/Hide Hidden Files"

[Elvis Angelaccio]

elvisangelaccio added a comment.


  Does it make sense to also create a standard action in kconfigwidgets?

REPOSITORY
  R237 KConfig

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

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

D28802: Add standard shortcut for "Show/Hide Hidden Files"

[Elvis Angelaccio]

elvisangelaccio added a comment.


  > and F8 seems kind of random.
  
  I was asking myself why F8 was chosen and yes, it looks like it was totally 
random: 
https://github.com/KDE/kdelibs/commit/8e26f7410a9280b504d97cdfbe5d3568cfa7b9d4

REPOSITORY
  R237 KConfig

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

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

D28535: [KIO-MTP] Fix null pointer dereference

[Elvis Angelaccio]

elvisangelaccio added a comment.


  In D28535#642298 , @feverfew wrote:
  
  > In D28535#642289 , 
@elvisangelaccio wrote:
  >
  > > In D28535#640833 , @fvogt 
wrote:
  > >
  > > > I assume there is a reason why `MTPDevice::getDevice()` has code for 
handling this very specific case, so I wouldn't just remove it without figuring 
out why: https://i.redd.it/hfnl7xo8yovy.gif
  > > >
  > > > If not, that would indeed be the best option.
  > >
  > >
  > > Unfortunately git blame doesn't seem to help us here.
  > >
  > > I suggest to push this fix to master only and see what happens.
  >
  >
  > By pushing this to master would we still be able to throw it up to 20.04.* 
if we decide it's stable enough? (also need to know to know what to put down as 
the FIXED-IN in the commit message)?
  
  
  Sure, we can cherry-pick that commit later.

REPOSITORY
  R320 KIO Extras

BRANCH
  fixNullPtr (branched from master)

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

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

D7563: Add privilegeExecution field to file protocol description

[Elvis Angelaccio]

elvisangelaccio added a comment.


  In D7563#645172 , @ngraham wrote:
  
  > The outstanding security issues have been resolved (see T8075 
). We have requested a re-review from the 
SUSE security team, but have not received it yet. Given that the original 
schedule for this feature has already slipped by almost two years, I would like 
to propose that we land this patch and turn it on, and resolve any 
newly-discovered issues in follow-up work.
  >
  > Any objections?
  
  
  +1

BRANCH
  master

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

To: chinmoyr, #frameworks, dfaure
Cc: feverfew, mreeves, mati865, ngraham, elvisangelaccio

D28535: [KIO-MTP] Fix null pointer dereference

[Elvis Angelaccio]

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


  In D28535#640833 , @fvogt wrote:
  
  > I assume there is a reason why `MTPDevice::getDevice()` has code for 
handling this very specific case, so I wouldn't just remove it without figuring 
out why: https://i.redd.it/hfnl7xo8yovy.gif
  >
  > If not, that would indeed be the best option.
  
  
  Unfortunately git blame doesn't seem to help us here.
  
  I suggest to push this fix to master only and see what happens.

REPOSITORY
  R320 KIO Extras

BRANCH
  fixNullPtr (branched from master)

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

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

D28520: Fix lifetime of slot in KIO-MTP

[Elvis Angelaccio]

elvisangelaccio accepted this revision.
elvisangelaccio added a comment.


  Please push to 20.04, there is still time.

REPOSITORY
  R320 KIO Extras

BRANCH
  slotLifetime (branched from master)

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

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

D27951: Allow users to change dropAction to MoveAction through kdeglobals

[Elvis Angelaccio]

elvisangelaccio added a comment.


  > With dndToMove=true in kdeglobals, drag files will move them without 
showing the menu. (holding Shift shows it)
  
  Are we going to expose this setting in Plasma?

REPOSITORY
  R241 KIO

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

To: trmdi, ngraham, dfaure, meven, #vdg, davidedmundson
Cc: elvisangelaccio, davidedmundson, meven, kde-frameworks-devel, LeGast00n, 
cblack, GB_2, michaelh, ngraham, bruns

D28060: Fix exitcode from kioexec when executable doesn't exist (and --tempfiles is set)

[Elvis Angelaccio]

elvisangelaccio added a comment.


  +1

REPOSITORY
  R241 KIO

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

To: dfaure, apol, davidedmundson, elvisangelaccio
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27591: KAbstractFileItemActionPlugin: Add missing quotes in code example

[Elvis Angelaccio]

elvisangelaccio accepted this revision.

REPOSITORY
  R241 KIO

BRANCH
  master

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

To: alex, elvisangelaccio, #frameworks, mlaurent, apol
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D26067: [KFilePlacesView] Add missing functionality required in order to be used by Dolphin again

[Elvis Angelaccio]

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


  Haven't tried the dolphin branch yet, just a comment on the API for now.
  From a quick look the patch looks good though.

INLINE COMMENTS

> kfileplacesview.h:68
> + */
> +bool showAll() const;
> +

This seems to be the name of a method that makes all items visible, not the 
name of a bool getter.

I know we already have `setShowAll()`, but we can deprecate it and find a 
better name. How about `setAllPlacesVisible()` / `allPlacesVisible()` / 
`allPlacesVisibleChanged()` ?

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg, #dolphin, elvisangelaccio
Cc: elvisangelaccio, meven, ngraham, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D26635: GSCreator: Fix hang due to calling exit() after fork()

[Elvis Angelaccio]

This revision was automatically updated to reflect the committed changes.
Closed by commit R373:3e2ea6e924d0: GSCreator: Fix hang due to calling exit() 
after fork() (authored by abizjak, committed by elvisangelaccio).

REPOSITORY
  R373 Image Thumbnailers

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26635?vs=73441=73898

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

AFFECTED FILES
  ps/gscreator.cpp

To: abizjak, #frameworks, elvisangelaccio
Cc: elvisangelaccio

D26635: GSCreator: Fix hang due to calling exit() after fork()

[Elvis Angelaccio]

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


  Looks safe enough to me.
  
  Can you provide an email address for the git authorship? Thanks!

REPOSITORY
  R373 Image Thumbnailers

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

To: abizjak, #frameworks, elvisangelaccio
Cc: elvisangelaccio

Re: Updating our coding conventions and coding style for C++11

[Elvis Angelaccio]

Hi,

On 16/01/20 18:46, Kai Uwe Broulik wrote:
> Hi,
> 
> for "auto" I think we should always annotate it with const, *, and/or &
> where appropriate:
> 
> auto *something = new MyCustomType;
> auto *keyEvent = static_cast(event);

IMHO the * here is redundant and only adds noise. It's clear that
`something` is a MyCustomType* and that `keyEvent` is a QKeyEvent*.

> auto  = foo[bar];

This could be helpful indeed.

> Cheers
> Kai Uwe

Cheers,
Elvis

D26484: Popup menu again to reposition it

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> dropjob.cpp:415
> +// popup again to update position. BUG: 415917
> +menu->popup(window ? window->mapToGlobal(m_relativePos) : 
> QCursor::pos());
>  }

`QCursor::pos()` should be avoided as it's not reliable on wayland.

The whole "popup() again" here looks like a workaround to me. Can't this be 
fixed on the plasma side?

REPOSITORY
  R241 KIO

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

To: trmdi, #frameworks, davidedmundson, elvisangelaccio, mart, dfaure
Cc: ngraham, broulik, anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D17595: Upstream Dolphin's file rename dialog

[Elvis Angelaccio]

elvisangelaccio accepted this revision.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D17595_2

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure, elvisangelaccio
Cc: elvisangelaccio, lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, 
bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham

D26277: KFilePlaceEditDialog: move logic into isIconEditable()

[Elvis Angelaccio]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:bdc2df9c1735: KFilePlaceEditDialog: move logic into 
isIconEditable() (authored by elvisangelaccio).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26277?vs=72341=72883

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

AFFECTED FILES
  src/filewidgets/kfileplaceeditdialog.cpp

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

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> krecentfilesmenu.cpp:31
> +QSettings *m_settings;
> +size_t m_maximumItems = 10;
> +};

Why not `int` since it's what we expose in the API anyway?

> krecentfilesmenu.cpp:53
> +setIcon(QIcon::fromTheme(QStringLiteral("document-open-recent")));
> +const QString fileName = 
> QStandardPaths::writableLocation(QStandardPaths::AppDataLocation) + 
> QLatin1String("/") + QCoreApplication::applicationName() + 
> QLatin1String("staterc");
> +d->m_settings = new QSettings(fileName, QSettings::Format::IniFormat, 
> this);

`QStringLiteral("...").arg()` would be more readable imho.

> krecentfilesmenu.h:32
> + *
> + * Replaces KRecentFilesAction from KConfigWidgets.
> + *

This is a porting guide, not sure it belongs in the main apidox of the class. 
(users of this new API may not know/care about KRecentFilesAction).

REPOSITORY
  R236 KWidgetsAddons

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

To: nicolasfella, #frameworks, dfaure
Cc: elvisangelaccio, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26358: KIO/SMB convert kio protocol declaration to json format

[Elvis Angelaccio]

elvisangelaccio added a comment.


  +1

REPOSITORY
  R320 KIO Extras

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

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

D17595: Upstream Dolphin's file rename dialog

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in renamefiledialog.h:48
> So, what should we rename `RenameFileDialog` to? `RenameFileDialog` or 
> `RenameFileOverwrittenDialog` ? ;)
> 
> Anyway, why wait for KF6 since this is new API? Can't we rename it now?

Ah sorry, I've read now the commit message. Which means there is a typo in this 
TODO:

"... and the class RenameDialog to RenameFileOverwrittenDialog or similar."

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D17595_1

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: elvisangelaccio, lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, 
bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham

D17595: Upstream Dolphin's file rename dialog

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> renamefiledialog.h:48
> + */
> +// TODO KF6  : rename the class RenameFileDialog to RenameDialog and the 
> class RenameFileDialog to RenameFileOverwrittenDialog or similar.
> +class KIOWIDGETS_EXPORT RenameFileDialog : public QDialog

So, what should we rename `RenameFileDialog` to? `RenameFileDialog` or 
`RenameFileOverwrittenDialog` ? ;)

Anyway, why wait for KF6 since this is new API? Can't we rename it now?

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D17595_1

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: elvisangelaccio, lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, 
bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham

D26277: KFilePlaceEditDialog: move logic into isIconEditable()

[Elvis Angelaccio]

elvisangelaccio added a comment.


  Ping?

REPOSITORY
  R241 KIO

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

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

D26277: KFilePlaceEditDialog: move logic into isIconEditable()

[Elvis Angelaccio]

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

REVISION SUMMARY
  Instead of relying on whether `m_iconButton` has been initialized
  somewhere else in the code.

TEST PLAN
  Make sure that editing icons is still allowed only for non-Trash places.

REPOSITORY
  R241 KIO

BRANCH
  isIconEditable (branched from master)

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

AFFECTED FILES
  src/filewidgets/kfileplaceeditdialog.cpp

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

D26276: KFilePlaceEditDialog: fix crash when editing the Trash place

[Elvis Angelaccio]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:a9ea007cf87c: KFilePlaceEditDialog: fix crash when 
editing the Trash place (authored by elvisangelaccio).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26276?vs=72339=72340

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

AFFECTED FILES
  src/filewidgets/kfileplaceeditdialog.cpp

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

D26276: KFilePlaceEditDialog: fix crash when editing the Trash place

[Elvis Angelaccio]

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

REVISION SUMMARY
  `m_iconButton` is initialized only when the url scheme of the place being
  edited is not `trash`.
  
  BUG: 415676
  FIXED-IN: 5.66

TEST PLAN
  Right-click the Trash place in dolphin and change the name.

REPOSITORY
  R241 KIO

BRANCH
  master

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

AFFECTED FILES
  src/filewidgets/kfileplaceeditdialog.cpp

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

D21695: Add FindTaglib.cmake

[Elvis Angelaccio]

elvisangelaccio added a comment.


  @heikobecker are you still interested in this patch? I can take over 
otherwise.

REPOSITORY
  R240 Extra CMake Modules

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

To: heikobecker, kde-buildsystem, kde-frameworks-devel, dfaure
Cc: ngraham, elvisangelaccio, cgiboudeaux, dfaure, LeGast00n, GB_2, bencreasy, 
michaelh, bruns

D26273: cmake: don't use taglib-config if we are cross compiling

[Elvis Angelaccio]

elvisangelaccio added a comment.


  In D26273#584234 , @vkrause wrote:
  
  > Longer term we probably either want to convince upstream to install cmake 
config files, or at least have this in ECM, a quick local grep find a handful 
of copies of this.
  
  
  There was an attempt with D21695  but it 
got stuck.

REPOSITORY
  R286 KFileMetaData

BRANCH
  bshah/cross

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

To: bshah, vkrause
Cc: elvisangelaccio, pino, kde-frameworks-devel, #baloo, hurikhan77, lots0logs, 
LeGast00n, fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D26197: Display fully qualified class/namespace name as page header

[Elvis Angelaccio]

elvisangelaccio added a reviewer: ochurlaud.

REPOSITORY
  R264 KApiDox

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

To: davidre, ochurlaud
Cc: ngraham, aacid, jucato, kde-frameworks-devel, kde-doc-english, LeGast00n, 
gennad, fbampaloukas, GB_2, michaelh, bruns, skadinna

D25683: KDirOperator: Use a fixed line height for scroll speed

[Elvis Angelaccio]

elvisangelaccio accepted this revision.

REPOSITORY
  R241 KIO

BRANCH
  diroperator_scrollspeed

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

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

D25720: Fix shortcut conflict between Cut and Delete File

[Elvis Angelaccio]

elvisangelaccio added a comment.


  In D25720#572291 , @ngraham wrote:
  
  > Though Dolphin seems to have some kind of local hack to make Shift+delete 
work for file deletion. But it doesn't work for Cut. It's all pretty messy.
  
  
  Yes it's messy but it's Microsoft fault since they chose to use an already 
used shortcut for another thing and we are doomed.
  
  Some years ago I submitted the very same patch and it eventually led to 
8eabbf6725386e716b7536c71a9181dfe5d959f0 
 in 
kxmlgui, which automatically handles the conflict for those few apps that need 
both actions (dolphin, gwenview, etc.)
  
  (Too bad reviewboard was shutdown, I had to dig in my mail to find it :/)
  
  So imho #414799 should be closed as intentional.

REPOSITORY
  R237 KConfig

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

To: ngraham, #frameworks, cfeck
Cc: elvisangelaccio, aspotashev, bcooksley, davidedmundson, aacid, apol, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25527: [activities/fileitemaction] Port plugin metadata to JSON

[Elvis Angelaccio]

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

REPOSITORY
  R320 KIO Extras

BRANCH
  json

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

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

Re: Bulk replacement of projects.kde.org on Frameworks modules

[Elvis Angelaccio]

On 11/11/19 22:52, Luigi Toscano wrote:
> Hi,
> basically all Frameworks components reference the ECM website
> using the old projects.kde.org URL, which is long gone and
> it is just a (partial) redirect.
> 
> See for example:
> 
> set_package_properties(ECM PROPERTIES TYPE REQUIRED
> DESCRIPTION "Extra CMake Modules."
> URL "https://projects.kde.org/projects/kdesupport/extra-cmake-modules;)
> 
> Can I go around a bulk-replace all the URLs with
> https://commits.kde.org/extra-cmake-modules, so that it would look like:
> 
> set_package_properties(ECM PROPERTIES TYPE REQUIRED
> DESCRIPTION "Extra CMake Modules."
> URL "https://commits.kde.org/extra-cmake-modules;)

+1

Using commits.kde.org like this feels weird though (i.e. using it
without a commit hash).

Something like https://code.kde.org/extra-cmake-modules would look much
better imho.

Just my 2c, maybe something that could be done after the move to gitlab.

> 
> There are few additional URLs which use the old website and I would replace
> them as well using the same pattern.
> 
> I'm asking to avoid ~70 almost-identical review requests.
> 
> Ciao
> 

Ciao
Elvis

Re: General Infrastructure Maintainability: API and EBN

[Elvis Angelaccio]

On 09/11/19 01:33, Ben Cooksley wrote:
> Hi all,

Hi Ben

> 
> Over the past number of years one of the tasks the Sysadmin team has
> worked on has been improving the overall maintainability of our
> systems, with a significant number of specialised cronjobs, exceptions
> and hidden linkages being eliminated.
> 
> That is with one great exception: api.kde.org and ebn.kde.org.
> 
> Both of these are suffering from an extreme amount of digital bitrot
> and special casing and in general are now in a condition where I
> cannot say for certain whether it would be possible to replicate the
> setup on a new system without us experiencing some degree of breakage
> (some of which we may not discover until weeks/months afterwards).
> 
> In addition, the current setup relies on an old-fashioned overnight
> reprocessing of all repositories, which is inefficient and resource
> expensive. A more modern approach would have the various projects api
> documentation generated on a delayed cycle from relevant branches as
> part of something like a CI job (but not part of the actual CI
> workflow itself).
> 
> For this one, i'm not certain on the best path forward at this stage,
> however the current state of affairs cannot continue. We have tried
> over the past few years to find people to work on a replacement for
> the tooling involved, but alas we've yet to have success here.
> 
> Thoughts anyone?

I'd say api.kde.org is too important to let it go. The EBN is less
important but still useful, I still see people pushing EBN-based fixes
once in a while.

Have we ever tried to use a GSoC project to modernize either of them? If
not, would it make sense to try next year?

> 
> Regards,
> Ben

Cheers,
Elvis

D24686: Replace usage of deprecated SlaveBase::config() by SlaveBase::configValue

[Elvis Angelaccio]

elvisangelaccio added a comment.


  In D24686#550533 , @dfaure wrote:
  
  > I never know when it's OK to increase the KF5 version dependency from 
applications modules, though.
  >
  > I have a ton of pending patches (to port away from KWindowSystem deprecated 
API) which require a KF5 version dep upgrade too...
  
  
  It's always ok to bump the KF5 version as long as you do it before the 
dependency freeze day set by the release team.

REPOSITORY
  R320 KIO Extras

BRANCH
  master

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

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

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Elvis Angelaccio]

elvisangelaccio added a reviewer: dfaure.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, ngraham, elvisangelaccio, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24773: kio_trash: Add size, modification, access and create date for trash:/

[Elvis Angelaccio]

elvisangelaccio added a comment.


  +1

INLINE COMMENTS

> trashimpl.cpp:1091
> +KIO::UDSEntry entry;
> +entry.clear();
> +

Is this really needed? We just created `entry`.

> trashimpl.cpp:1094
> +// refresh list of trashes and get the list of files in them
> +const TrashedFileInfoList fileInfoList = list();
> +

coding style: local variables never start with an uppercase.

REPOSITORY
  R241 KIO

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

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

Re: Taking over maintainership of the kaccounts-* repos

[Elvis Angelaccio]

On 13/09/19 10:21, Bhushan Shah wrote:
> Hello!
> 
> At akademy we talked about the KAccounts and realized that kaccounts is
> unmaintained and/or Martin is not active.
> 
> So I propose to take over maintainership of the following repositories.
> 
> - kaccounts-integration
> - kaccounts-providers
> - kaccounts-mobile
> 
> If you have any concerns/objections with this, let me know.

Hi Bhushan,

would you be ok with setting yourself as the default assignee for
kaccounts bugs?

Currently the default assignee is kde-telepathy-b...@kde.org, which I'm
afraid means no one is looking at those bugs.


> 
> Thanks
> 

Thanks,
Elvis

D7446: [Places panel] Revamp the Recently Saved section

[Elvis Angelaccio]

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

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D7446

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

To: meven, #dolphin, broulik, elvisangelaccio, #vdg, #frameworks, ngraham
Cc: meven, trickyricky26, andreask, huftis, svenmauch, kde-frameworks-devel, 
spoorun, andreaska, gregormi, markg, alexeymin, broulik, elvisangelaccio, 
dfaure, davidedmundson, ltoscano, #konqueror, iasensio, fprice, LeGast00n, 
MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, michaelh, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D21897: Address some issues reported by Krazy analysis

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> pajamapants3000 wrote in ktextedit.cpp:373
> Thank you broulik! Could we use a `QScopedPointer` here? I see them 
> throughout the KDE source, but not often for dialogs. The krazy analysis 
> looked for `QPointer`, referencing blog https://blogs.kde.org/node/3919, 
> which appears to have predated the existence of `QScopedPointer`.

While we are at it, the best way to fix this would be to not use `exec()` at 
all, but use `open()` instead: 
https://wiki.qt.io/New_Signal_Slot_Syntax#Asynchronous_made_easier

REPOSITORY
  R310 KTextWidgets

BRANCH
  krazy-50b2564 (branched from master)

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

To: pajamapants3000, elvisangelaccio
Cc: broulik, vkrause, elvisangelaccio, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D23961: Add apidox to most of KFilePlacesModel

[Elvis Angelaccio]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:bcf51ae68193: Add apidox to most of KFilePlacesModel 
(authored by elvisangelaccio).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23961?vs=66124=66149

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

AFFECTED FILES
  src/filewidgets/kfileplacesmodel.h

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

D23955: KBookmark: improve addBookmark apidox

[Elvis Angelaccio]

This revision was automatically updated to reflect the committed changes.
Closed by commit R294:797763d4b4d2: KBookmark: improve addBookmark apidox 
(authored by elvisangelaccio).

REPOSITORY
  R294 KBookmarks

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23955?vs=66108=66148

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

AFFECTED FILES
  src/kbookmark.h

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

D23961: Add apidox to most of KFilePlacesModel

[Elvis Angelaccio]

elvisangelaccio updated this revision to Diff 66124.
elvisangelaccio added a comment.


  Mention that URLs will be stored in the QUrl::FullyEncoded string format.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23961?vs=66121=66124

BRANCH
  master

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

AFFECTED FILES
  src/filewidgets/kfileplacesmodel.h

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

D23961: Add apidox to most of KFilePlacesModel

[Elvis Angelaccio]

elvisangelaccio updated this revision to Diff 66121.
elvisangelaccio added a comment.


  - Fixed typo

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23961?vs=66118=66121

BRANCH
  master

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

AFFECTED FILES
  src/filewidgets/kfileplacesmodel.h

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

D23961: Add apidox to most of KFilePlacesModel

[Elvis Angelaccio]

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

REPOSITORY
  R241 KIO

BRANCH
  master

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

AFFECTED FILES
  src/filewidgets/kfileplacesmodel.h

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

D23956: KFilePlacesModel: fix @since tags

[Elvis Angelaccio]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:90c5872c08de: KFilePlacesModel: fix @since tags (authored 
by elvisangelaccio).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23956?vs=66109=66111

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

AFFECTED FILES
  src/filewidgets/kfileplacesmodel.h

To: elvisangelaccio, dfaure, aacid
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23956: KFilePlacesModel: fix @since tags

[Elvis Angelaccio]

elvisangelaccio added a comment.


  Yeah, that's my next patch ;)

REPOSITORY
  R241 KIO

BRANCH
  master

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

To: elvisangelaccio, dfaure, aacid
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23956: KFilePlacesModel: fix @since tags

[Elvis Angelaccio]

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

REVISION SUMMARY
  A bunch of @since tags were missing or wrong.
  While at it, use the same ///< style for enum apidox comments.

REPOSITORY
  R241 KIO

BRANCH
  master

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

AFFECTED FILES
  src/filewidgets/kfileplacesmodel.h

To: elvisangelaccio, dfaure, aacid
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23955: KBookmark: improve addBookmark apidox

[Elvis Angelaccio]

elvisangelaccio added a comment.


  In D23955#531647 , @dfaure wrote:
  
  > OK (why does this matter to the user of the class, though?)
  
  
  See D23706 , dolphin is going to need 
regex-based matches on the placesmodel URLs.

REPOSITORY
  R294 KBookmarks

BRANCH
  master

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

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

D23955: KBookmark: improve addBookmark apidox

[Elvis Angelaccio]

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

REVISION SUMMARY
  Clarify that the URL will be stored in its toString(QUrl::FullyEncoded)
  version.

REPOSITORY
  R294 KBookmarks

BRANCH
  master

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

AFFECTED FILES
  src/kbookmark.h

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

D23851: Avoid sending KDirNotify::emitFilesAdded when the emptytrashjob finishes

[Elvis Angelaccio]

elvisangelaccio added a comment.


  +1
  
  In D23851#529463 , @ngraham wrote:
  
  > If the on;y thing that `EmptyTrashJob::slotFinished()` does now is call 
`SimpleJob::slotFinished()`, do we even still need to override the base class's 
implementation of `slotFinished()`?
  
  
  Yeah we don't, but we can't remove it because it would be a BIC.

REPOSITORY
  R241 KIO

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

To: meven, broulik, ngraham
Cc: elvisangelaccio, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21795: [KAuth] Add support for action details in Polkit1 backend.

[Elvis Angelaccio]

elvisangelaccio added a comment.


  polkit-qt-1-0.113.0 has been released, so I believe now we can move over with 
this patch.

REPOSITORY
  R283 KAuth

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

To: chinmoyr, apol, bruns, davidedmundson, #frameworks, dfaure, cfeck, sitter
Cc: elvisangelaccio, bcooksley, ngraham, sitter, mreeves, kde-frameworks-devel, 
LeGast00n, GB_2, michaelh, bruns

D7446: [Places panel] Revamp the Recently Saved section

[Elvis Angelaccio]

elvisangelaccio added a comment.


  LGTM besides the inline nitpicks.

INLINE COMMENTS

> kfileplacesmodeltest.cpp:686
>  KBookmark device = root.first(); // The device we'll move is the 6th 
> bookmark
> -for (int i = 0; i < 5; i++) {
> +int stop = m_hasRecentlyUsedKio ? 7 : 5;
> +for (int i = 0; i < stop; i++) {

`const`, and maybe a more descriptive name (`count` or similar?)

> kfileplacesmodeltest.cpp:967
>  // places
> -QTest::newRow("Places - Home") << m_places->index(0, 0)
> +int idx = 0;
> +QTest::newRow("Places - Home") << m_places->index(idx++, 0)

I'd avoid cryptic names. If we can't use `index`, maybe just `i`?

REPOSITORY
  R241 KIO

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

To: meven, #dolphin, broulik, elvisangelaccio, #vdg, #frameworks, ngraham
Cc: meven, trickyricky26, andreask, huftis, svenmauch, kde-frameworks-devel, 
spoorun, andreaska, gregormi, markg, alexeymin, broulik, elvisangelaccio, 
dfaure, davidedmundson, ltoscano, #konqueror, iasensio, fprice, LeGast00n, 
MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, michaelh, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D23898: [KUrlNavigatorButton] Fix QString usage to not use [] out of bounds.

[Elvis Angelaccio]

elvisangelaccio accepted this revision.
elvisangelaccio added a comment.


  Everything looks good and the new test passes for me.

REPOSITORY
  R241 KIO

BRANCH
  master

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

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

D23898: [KUrlNavigatorButton] Fix QString usage to not use [] out of bounds.

[Elvis Angelaccio]

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


  I don't have 5.14 around atm, so I trust you've tested this :)

INLINE COMMENTS

> kurlnavigatortest.cpp:265
> +KFilePlacesModel model;
> +const QString pwd = QDir::currentPath();
> +const QUrl url = QUrl::fromLocalFile(pwd);

Did you mean `cwd` ?

REPOSITORY
  R241 KIO

BRANCH
  master

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

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

D21897: Address some issues reported by Krazy analysis

[Elvis Angelaccio]

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

INLINE COMMENTS

> vkrause wrote in krichtextwidget.cpp:638
> This is unusual, but it's the right way around. The point of this is to check 
> if the dialog has been deleted during the sub-eventloop behind exec(). No 
> idea if that can happen here though, but that's generally the idea behind 
> this pattern.

Ah of course. I didn't notice it was `exec()` that was being called.

REPOSITORY
  R310 KTextWidgets

BRANCH
  krazy-50b2564 (branched from master)

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

To: pajamapants3000, elvisangelaccio
Cc: vkrause, elvisangelaccio, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D21897: Address some issues reported by Krazy analysis

[Elvis Angelaccio]

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

INLINE COMMENTS

> krichtextwidget.cpp:638
>  
> -if (linkDialog->exec()) {
> +if (linkDialog->exec() && linkDialog) {
>  q->updateLink(linkDialog->linkUrl(), linkDialog->linkText());

This should be `linkDialog && linkDialog->exec()`

> ktextedit.cpp:369
>  }
> -if (dialog.exec()) {
> -setSpellCheckingLanguage(dialog.language());
> +if (dialog->exec() && dialog) {
> +setSpellCheckingLanguage(dialog->language());

Same here, we need to check the pointer before using it.

REPOSITORY
  R310 KTextWidgets

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

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

D23201: [KDirOperator] Middle-elide labels that are too long to fit

[Elvis Angelaccio]

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

REPOSITORY
  R241 KIO

BRANCH
  middle-elide-kdiroperator-labels (branched from master)

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

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

D22265: KPluginMetaData: use Q_DECLARE_METATYPE

[Elvis Angelaccio]

elvisangelaccio added a comment.


  In D22265#491249 , @kossebau wrote:
  
  > This runs the chance to break some 3rd-party software which also calls the 
Q_DECLARE_METATYPE(KPluginMetaData) macro.
  >  Any chance this could be moved to the place that needs this for now, and 
perhaps add a KF6 TODO instead?
  >
  > See e.g. R36:3aacbbaab50ac6e0557c5d69c430459eb3d71ad7 
 
which was needed to unbreak ark builds.
  
  
  Ping?

REPOSITORY
  R244 KCoreAddons

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

To: apol, #frameworks, mart
Cc: elvisangelaccio, kossebau, kde-frameworks-devel, LeGast00n, sbergeron, 
michaelh, ngraham, bruns

D21782: Add a warning dialog with details, continue, and cancel button

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> kmessagebox.h:329
> + *
> + * @since 5.60
> + */

Needs a bump to 5.61

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  master

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

To: chinmoyr, #vdg, #frameworks, dfaure, ngraham, apol
Cc: elvisangelaccio, mreeves, ngraham, GB_2, kde-frameworks-devel, LeGast00n, 
sbergeron, michaelh, bruns

D22144: - Add kio recentlyused:/ to access KactivitytStats data

[Elvis Angelaccio]

elvisangelaccio added a comment.


  +1

REPOSITORY
  R320 KIO Extras

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

To: meven, ivan, #frameworks, ngraham
Cc: elvisangelaccio, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, 
fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, navarromorales, 
firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22144: - Add kio recentlyused:/ to access KactivitytStats data

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> recentlyused.protocol:1-14
> +[Protocol]
> +X-DocPath=kioslave5/recentlyused/index.html
> +exec=kf5/kio/recentlyused
> +protocol=recentlyused
> +Icon=document-open-recent
> +input=none
> +output=filesystem

We should use the JSON format for new ioslaves.

You can use the slaves in KIO as example.

REPOSITORY
  R320 KIO Extras

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

To: meven, ivan, #frameworks, ngraham
Cc: elvisangelaccio, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, 
fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, navarromorales, 
firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D15739: [Places panel] Don't show Root by default

[Elvis Angelaccio]

elvisangelaccio accepted this revision.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D15739

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

To: meven, #dolphin, #vdg, tcanabrava, ngraham, elvisangelaccio
Cc: meven, elvisangelaccio, Codezela, davidc, tcanabrava, ndavis, romangg, 
bruns, davidedmundson, abetts, svenmauch, broulik, acrouthamel, 
kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21695: Add FindTaglib.cmake

[Elvis Angelaccio]

elvisangelaccio added a comment.


  Where does this FindTaglib.cmake come from?
  
  We should probably use the one from kio-extras, which got a bunch of fixes in 
the last months.

REPOSITORY
  R240 Extra CMake Modules

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

To: heikobecker, kde-buildsystem, kde-frameworks-devel, dfaure
Cc: elvisangelaccio, cgiboudeaux, dfaure, LeGast00n, bencreasy, michaelh, 
ngraham, bruns

D21695: Add FindTaglib.cmake

[Elvis Angelaccio]

elvisangelaccio added a comment.


  In D21695#476826 , @heikobecker 
wrote:
  
  > I'm not entirely sure about taglib-config on Windows
  
  
  See 
https://cgit.kde.org/kio-extras.git/commit/cmake/FindTaglib.cmake?id=548f525f4308810888c85f42a570139029c40618

REPOSITORY
  R240 Extra CMake Modules

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

To: heikobecker, kde-buildsystem, kde-frameworks-devel, dfaure
Cc: elvisangelaccio, cgiboudeaux, dfaure, LeGast00n, bencreasy, michaelh, 
ngraham, bruns

D21367: kioexec: change the scary debug messages for delayed deletion

[Elvis Angelaccio]

elvisangelaccio accepted this revision.

REPOSITORY
  R241 KIO

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

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

D21367: kioexec: change the scary debug messages for delayed deletion

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> main.cpp:262
>  // it will have time to start up and read the file before it 
> gets deleted. #130709.
> -qDebug() << "sleeping...";
> -QThread::currentThread()->sleep(180); // 3 mn
> +const int sleep_secs = 180;
> +qDebug() << "sleeping for" << sleep_secs << "seconds before 
> deleting file...";

Please use camelCase

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

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

D21367: kioexec: change the scary debug messages for delayed deletion

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> main.cpp:261-263
>  // it will have time to start up and read the file before it 
> gets deleted. #130709.
> -qDebug() << "sleeping...";
> +qDebug() << "sleeping for 3 minutes before deleting file...";
>  QThread::currentThread()->sleep(180); // 3 mn

While at it, maybe it could be worth to put 180 in some variable `foo` and then 
use `foo/60` instead of hardcoding the number of minutes in the debug message.

REPOSITORY
  R241 KIO

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

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

Re: Symmy in kde-review

[Elvis Angelaccio]

On 27/04/19 18:46, Ingo Klöcker wrote:
> On Montag, 22. April 2019 17:57:42 CEST Elvis Angelaccio wrote:
>> On 12/04/19 13:12, Adriaan de Groot wrote:
>>> # Runtime
>>>
>>> Since it's supposed to be a CLI application, you might want to massage the
>>> QPA loading a bit, since when I run it (ssh'ed in to my build machine) It
>>> does
>>> this:
>> Note that even if it can be used from CLI, it's still a GUI app as it
>> needs Qt Widgets to show the passphrase dialogs.
> 
> Hmm. I'm wondering why you don't leave the passphrase handling to gpg (resp. 
> gpg-agent) which will show a passphrase dialog (via one of the pinentry 
> implementations) if necessary. One of the major design goals of the gpg2 
> architecture was that passphrase and secret key handling is done by gpg-agent 
> exclusively.

Hi Ingo,

the passphrase handling is still done by gpg-agent. gpgme allows to use
a custom dialog instead of pinentry, which is what I've done to achieve
better integration with Plasma.

Cheers,
Elvis


> 
> Regards,
> Ingo
> 

D21249: Test current filter before setting a new one

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> kfilewidget.cpp:132
>  void updateLocationEditExtension(const QString &);
> +bool matchFilter(const QString , const QString , const 
> bool bUpdate);
>  void updateFilter();

Nitpick: I'd make `filter` the first parameter. And we usually don't use `const 
boll` in signatures, but just `bool`.

`bUpdate` doesn't tell the reader what this variable is used for. How about 
calling it `updateCurrentFilter` or similar?

REPOSITORY
  R241 KIO

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

To: jglogowski, #frameworks, ngraham
Cc: elvisangelaccio, ngraham, michaelweghorn, kde-frameworks-devel, michaelh, 
bruns

D21249: Test current filter before setting a new one

[Elvis Angelaccio]

elvisangelaccio added a reviewer: dfaure.

REPOSITORY
  R241 KIO

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

To: jglogowski, #frameworks, ngraham, dfaure
Cc: elvisangelaccio, ngraham, michaelweghorn, kde-frameworks-devel, michaelh, 
bruns

D20838: Allow to drop one file or one folder on KDirOperator

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> kfilewidgettest.cpp:38
> +#include 
> +#include 
> +

Unused?

> kfilewidgettest.cpp:407-413
> +QDir f(tempDir.filePath(dir));
> +
> +KFileWidget fw(QUrl::fromLocalFile(tempDir.path()));
> +fw.setOperationMode(KFileWidget::Opening);
> +fw.setMode(KFile::File);
> +fw.show();
> +fw.activateWindow();

Please use descriptive variable names instead of `f` and `fw`

> kfilewidgettest.cpp:415
> +
> +QList list_urls = {fileUrl};
> +QMimeData *mimeData = new QMimeData();

Missing camelCase

> kdiroperator.cpp:1404-1405
> +
> +QMimeDatabase md;
> +QMimeType mt = md.mimeTypeForUrl(url);
> +

Please use descriptive variable names also here.

> kdiroperator.cpp:1407
> +
> +QRegExp r;
> +r.setPatternSyntax(QRegExp::Wildcard);   // the "mimetype" 
> can be "image/*"

QRegExp should be avoid in new code, can you try to use QRegularExpression 
instead?

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D20838_1

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

To: meven, ngraham, #frameworks
Cc: elvisangelaccio, apol, kde-frameworks-devel, michaelh, ngraham, bruns

D20964: [FileWidget] Replace "Filter:" with "File type:" when saving with a limited list of mimetypes

[Elvis Angelaccio]

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


  Please wait the tagging before pushing.

REPOSITORY
  R241 KIO

BRANCH
  file-type-when-saving-and-mimetype-is-defined (branched from master)

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

To: ngraham, #vdg, elvisangelaccio, GB_2
Cc: apol, kde-frameworks-devel, michaelh, ngraham, bruns

D20827: Reword some text, a couple clean ups, add a separator

[Elvis Angelaccio]

elvisangelaccio added a comment.


  In D20827#45 , @meven wrote:
  
  > In D20827#457391 , 
@elvisangelaccio wrote:
  >
  > > @meven FYI this change broke the Frameworks string freeze: 
https://community.kde.org/Schedules/Frameworks
  >
  >
  > Should I revert this then ?
  
  
  Someone could have already started to translate these new strings. Just keep 
it in mind for the next time ;)

REPOSITORY
  R241 KIO

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

To: meven, ngraham, #frameworks, #vdg, GB_2
Cc: elvisangelaccio, kde-frameworks-devel, cblack, arvidhansson, ian, jguidon, 
hannahk, Pixel_Lime, jraleigh, squeakypancakes, alexde, IohannesPetros, GB_2, 
trickyricky26, mglb, michaelh, crozbo, ndavis, firef, ngraham, bruns, skadinna, 
aaronhoneycutt, mbohlender

D20938: Add Mounts Backend

[Elvis Angelaccio]

elvisangelaccio added reviewers: broulik, bruns.

REPOSITORY
  R245 Solid

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

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

D20934: Rename file dialog "Filter" label text to "Type"

[Elvis Angelaccio]

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


  "Filter" is the correct word, you can enter any string in the label (not 
necessarily a file type) and the view will be filtered accordingly.

REPOSITORY
  R241 KIO

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

To: GB_2, #frameworks, #plasma, #vdg, elvisangelaccio
Cc: elvisangelaccio, #vdg, #plasma, kde-frameworks-devel, #frameworks, 
michaelh, ngraham, bruns