D29434: Use small font for ExpandableListItem subtitle

[Noah Davis]

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

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  ExpandableListItem-small-subtitle-font (branched from master)

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

To: ngraham, #plasma, #vdg, broulik, ndavis
Cc: cblack, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D25814: [KColorScheme] Add SeparatorColor

[Nathaniel Graham]

ngraham added a comment.


  I have to agree with @ndavis here. Adding a separator color to the color 
scheme seems sensible enough to me, for the reasons previously provided.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: broulik, manueljlin, alexde, ngraham, davidedmundson, filipf, cfeck, 
hpereiradacosta, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis added a comment.


  In D25814#598380 , @cfeck wrote:
  
  > > Why don't focus and hover colors belong?
  >
  > Because an application cannot know if and how a style does indicate focus 
or hovering.
  
  
  I don't understand this objection. What would allow an application to know 
about the button background from the QStyle? It could be a PNG, and yet setting 
the button background color is perfectly fine.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: broulik, manueljlin, alexde, ngraham, davidedmundson, filipf, cfeck, 
hpereiradacosta, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D29454: [TaglibExtractor] Add support for Audible "Enhanced Audio" audio books

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:0975ed45af79: [TaglibExtractor] Add support for Audible 
Enhanced Audio audio books (authored by bruns).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29454?vs=82017=82044

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

AFFECTED FILES
  autotests/samplefiles/no-meta/test.aax
  autotests/samplefiles/nocoverage_test.aax
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.json

To: bruns, #frameworks, #baloo, ngraham, astippich, mgallien
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29463: Fix Kirigami.Units.devicePixelRatio=1.3 when it should be 1.0 at 96dpi

[Chris Holland]

Zren added reviewers: Kirigami, mart.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: Zren, #kirigami, mart
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29463: Fix Kirigami.Units.devicePixelRatio=1.3 when it should be 1.0 at 96dpi

[Chris Holland]

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

REVISION SUMMARY
  I recently noticed that `Kirigami.Units.devicePixelRatio` was `1.3` even 
though I was at the default 96dpi when writing a widget's config.
  
  F8287886: 2020-05-05___19-00-48.png 
  
  There's 4 different `Units.qml`, and I'm not certain which does what. 2 are 
in `krigiami` and the other 2 are in `plasma-framework`.
  
$ locate /Units.qml | grep /usr/
/usr/lib/qt/qml/org/kde/kirigami.2/Units.qml
/usr/lib/qt/qml/org/kde/kirigami.2/styles/Plasma/Units.qml
/usr/lib/qt/qml/org/kde/kirigami.2/styles/org.kde.desktop/Units.qml
/usr/lib/qt/qml/org/kde/kirigami.2/styles/org.kde.desktop.plasma/Units.qml
  
  
  
  - 2 years ago `kirigami/src/controls/Units.qml` 
 
(`/usr/lib/qt/qml/org/kde/kirigami.2/Units.qml`) was changed from `13/10 = 1.3` 
to multiply by `13*0.75` so that `9.75/10 = 0.975` (then `max(1, 0.975) = 1`).
  
  -
  
  There's 2 files owned by `plasma-framework`. The `/styles/Plasma/Units.qml` 
binds PlasmaCore's `units.devicePixelRatio`, so that leaves 
`/styles/org.kde.desktop.plasma/Units.qml`.
  
$ pacman -Qo 
/usr/lib/qt/qml/org/kde/kirigami.2/styles/org.kde.desktop.plasma/Units.qml
/usr/lib/qt/qml/org/kde/kirigami.2/styles/org.kde.desktop.plasma/Units.qml 
is owned by plasma-framework 5.69.0-2
  
  This patch edits the `org.kde.desktop.plasma` style, and fixes `qmlscene 
KirigamiDevicePixelRatioTest.qml`, and `plasmashell` widget configs, and 
`plasmoidviewer` widget configs.
  
  The Kirigami patch is D29462 

TEST PLAN
  You can edit 
`/usr/lib/qt/qml/org/kde/kirigami.2/styles/org.kde.desktop.plasma/Units.qml` 
then run:
  
  - https://gist.github.com/Zren/621338b8cda7c550d7b43f8ea1ba71a7
  - `qmlscene KirigamiDevicePixelRatioTest.qml`
  
  and `Kirigami.Units.devicePixelRatio` should equal `1`.

REPOSITORY
  R242 Plasma Framework (Library)

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

AFFECTED FILES
  src/declarativeimports/kirigamiplasmadesktopstyle/Units.qml
  src/declarativeimports/kirigamiplasmastyle/Units.qml

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

D29461: Fix kio-extras build on Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 82039.
brute4s99 added a comment.


  needs testing on windows, will update in a while

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29461?vs=82036=82039

BRANCH
  arcpatch-D29461

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

AFFECTED FILES
  sftp/CMakeLists.txt
  sftp/kio_sftp.cpp

To: brute4s99, vonreth
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

D29461: Fix kio-extras build on Windows

[Piyush Aggarwal]

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

REPOSITORY
  R320 KIO Extras

BRANCH
  master

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

AFFECTED FILES
  sftp/CMakeLists.txt
  sftp/kio_sftp.cpp

To: brute4s99, vonreth
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

Re: Adding the Abstract Games Suite to the KDE Applications

[Carl Schwan]

Hi,
the developers are both minors but the big brother who is a regular of 
#kde-chat and #kde-vdg already disclosed his name but he also felt 
uncomfortable disclosing the name of his little brother without his 
authorization and I agree with him. We shouldn't ask minors to disclose their 
names.

With the games being under BSD-2 now, do we really need to have the names of 
the developers? It's not like we can't relicense the code to another license if 
we wanted. 


Aside from this issue that I hope Ingo can help with, I'm in favor of 
incubating Abstract Games. The games are great, they could probably work on 
Plasma Mobile with a bit of work. Also, the developers are talented as seen in 
one of their other projects a basic Qt office suite: 
https://gitlab.com/abstractsoftware/o20/o20coreapps. 


I volunteer to be the sponsor or one of the sponsors and help them to migrate 
to the KDE infrastructure.

Regards,
Carl
‐‐‐ Original Message ‐‐‐
Le mardi, mai 5, 2020 9:37 PM, Albert Astals Cid  a écrit :

> El divendres, 1 de maig de 2020, a les 23:37:56 CEST, The Abstract Developers 
> va escriure:
> 

> > Hello!
> > In https://gitlab.com/abstractsoftware/abstract-games/-/issues/1, we were 
> > invited to add the Abstract Games suite into the KDE Applications, and of 
> > course we couldn't refuse!
> > We are a small team of two, who would prefer to remain anonymous if that's 
> > OK, and we are hosted on free software (Gitlab) at 
> > https://gitlab.com/abstractsoftware/abstractgames. I believe that all 
> > aspects of the KDE Manifesto are satisfied.
> 

> About remaining anonymous, that's a more picky subject and we are not sure 
> it's a yes or it's a no yet.
> 

> Not sure what to tell you here to be honest.
> 

> Ingo you mentioned you'd work on that, do we have an ETA?
> 

> Cheers,
> Albert
> 

> > Thank you,
> > the Abstract Developers



publickey - carl@carlschwan.eu - 0x7F564CB5.asc
Description: application/pgp-keys


signature.asc
Description: OpenPGP digital signature

D29420: Generate DBus interface

[Nicolas Fella]

nicolasfella added inline comments.

INLINE COMMENTS

> broulik wrote in notifybypopup.cpp:115
> Previously it would accept the signal from any service which I find odd, 
> though. Could you maybe check git logs to see if there was a reason for this?
> It should survive restarts anyway and the service name is defined, so I 
> really don't see why it used to be a blind connect.

git history until the frameworks split isn't really telling, I didn't bother 
looking further back

REPOSITORY
  R289 KNotifications

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

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

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Benjamin Port]

bport planned changes to this revision.

REPOSITORY
  R237 KConfig

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

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

Re: Adding the Abstract Games Suite to the KDE Applications

[Albert Astals Cid]

El divendres, 1 de maig de 2020, a les 23:37:56 CEST, The Abstract Developers 
va escriure:
> Hello!
> In https://gitlab.com/abstractsoftware/abstract-games/-/issues/1, we were 
> invited to add the Abstract Games suite into the KDE Applications, and of 
> course we couldn't refuse!
> We are a small team of two, who would prefer to remain anonymous if that's 
> OK, and we are hosted on free software (Gitlab) at 
> https://gitlab.com/abstractsoftware/abstractgames. I believe that all aspects 
> of the KDE Manifesto are satisfied.

About remaining anonymous, that's a more picky subject and we are not sure it's 
a yes or it's a no yet.

Not sure what to tell you here to be honest.

Ingo you mentioned you'd work on that, do we have an ETA?

Cheers,
  Albert

> Thank you,
> 
> the Abstract Developers
> 

D29454: [TaglibExtractor] Add support for Audible "Enhanced Audio" audio books

[Stefan Brüns]

bruns added a comment.


  In D29454#664222 , @ngraham wrote:
  
  > Nice!
  
  
  and when you combine it with 
https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/577, 
Elisa can play Audible audio books ;-)

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

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

To: bruns, #frameworks, #baloo, ngraham, astippich, mgallien
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29363: Use UI marker context in more tr() calls

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Thanks Albert (& Luigi) for review. With no-one objecting, I would proceed to 
push this upcoming WE, 9/10th May, to be early in the cycle still.

REPOSITORY
  R236 KWidgetsAddons

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

To: kossebau, #frameworks, #localization, cfeck
Cc: aacid, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

Re: Mentors needed

[Valorie Zimmerman]

He cannot officially stay part of GSoC under those conditions. 29th June is
First Evaluations by which time he should have finishing roughly one third
of his project.

Please ask him urgently to withdraw. We're waiting on the story
announcing and welcoming the accepted students until that is done.

We can discuss what can be done unofficially and out of GSoC when he has
the time to do the work.

Valorie

On Tue, May 5, 2020 at 6:51 AM Alexa M  wrote:

> Hi,
>
> Yeah, he's the same student. He says that he could contribute only a
> little until his university exams end date and after that full-time. That
> could mean part or very little time until 10-20 of July and 100% the rest
> of the time, However there is still a possibility that the exams
> dates shift but he can't say for sure as there is no official
> announcement yet regarding the final dates. Concluding, he said that if
> this is ok for us then he would also be ok to continue under these
> conditions.
>
> Best,
> Alexandra
>
> Στις Τρί, 5 Μαΐ 2020 στις 5:45 π.μ., ο/η Valorie Zimmerman <
> valorie.zimmer...@gmail.com> έγραψε:
>
>> Are we discussing the same student, Agisilaos Kounelis who proposed "Port
>> QtQuickControls Calendar widget to QtQuickControls2 module" ? If so, they
>> have not withdrawn. If this indeed the student, if they will not
>> participate officially then need to *now* mark "Withdraw."
>>
>> Valorie
>>
>> On Mon, May 4, 2020 at 2:12 AM Alexa M  wrote:
>>
>>> Hi,
>>>
>>> The student decided to not to participate in GSoC this year due to other
>>> university obligations running late because of the current situation
>>> including uncertain exams dates and he was afraid he would not be able
>>> to comply.
>>>
>>> Best,
>>> AlexandraB
>>>
>>> On Fri, May 1, 2020, 00:06 Timothée Giet  wrote:
>>>
 Le 30/04/2020 à 22:07, Timothée Giet a écrit :

 Le 30/04/2020 à 21:38, Valorie Zimmerman a écrit :

 Hello folks, we have a project about porting a QtQuickControls module
 to QtQuickControls2 with only one mentor who marked "I want to mentor" on
 the GSoC webapp.

 If you have already signed into the GSoC webapp as a mentor, and would
 like to help out with this project, please mark "I want to mentor" in the
 app.

 If you have the skills needed to mentor but have not yet logged into
 the app, please:

 1. ensure that you are subscribed to KDE-Soc-Mentor mail list [1]

 2. write to kde-soc-managem...@kde.org and ask to be invited to the
 webapp.

 Thanks!

 Valorie

 1. https://mail.kde.org/mailman/listinfo/kde-soc-mentor

 --
 http://about.me/valoriez - pronouns: she/her


 I may not be the best mentor this project, but as GCompris would
 benefit a lot from the output of this proposal (this missing module is the
 main blocker we have to migrate to QtQuickControls2), I'm ok to
 backup-mentor it... At least I can provide generic advices and testing.

 But some help from someone more specialist of Qt internals would be
 welcome!

 Timothée

 On second thought, as I'm not so sure I can provide valuable help to
 mentor this project, I unchecked the "want to mentor" button there... but
 again, if there's anyone else willing to help mentoring this project, it
 would be awesome.

 Timo.

>>>
>>
>> --
>> http://about.me/valoriez - pronouns: she/her
>>
>>
>>

-- 
http://about.me/valoriez - pronouns: she/her

D29017: WIP: Introduce three new methods that return all "entries" in a folder

[Mikołaj Płomieński]

blaze added a comment.


  Check this commit message 
https://phabricator.kde.org/R32:f56cdda54b7f88b119f2c7cfb2f534b4fe74478f

REPOSITORY
  R311 KWallet

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

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

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Benjamin Port]

bport added a comment.


  In D28221#657482 , @dfaure wrote:
  
  > Ah! Now it actually makes sense to me. If we are changing what 
revertToDefault() does, then it makes sense to change the if() condition for 
calling it. Basically, now that it does the right thing in both cases (default 
from C++ and default from system file) it can be called in both cases, while 
before it was only called if there was no default from a system file. Right?
  >
  > I'm still wondering about this though: "If we're saving a value equal to 
the C++ default, then we have to write it out, otherwise upon reading the 
global-file default will interfer."
  >  Your unittest shows that revertToDefault() will lead to the global-file 
value being read.
  >  Doesn't this mean that this code is wrong then?
  >
  >if (value == "cppdefault") {
  >   cg.revertToDefault(key);
  >   } else {
  >   cg.writeEntry(key, value);
  >   }
  >
  > That's the code you're using everywhere, so that's actually the code that 
would be good to unittest ;-)
  >  Isn't this buggy when the value actually *is* cppdefault, and there is a 
system config file with another default value?
  >
  > > Indeed probably need to update configuration too.
  >
  > Did you mean documentation? I know this is all about configuration ;) but 
the bit that's missing is to update the documentation, and add a task to get 
rid of the hasDefault() everywhere before revertToDefault is called. Assuming 
I'm wrong above about there being a bug left :-)
  
  
  
  
  In D28221#664243 , @bport wrote:
  
  > - Fix comments
  > - Ensure we don't have problem if we set value to "defaultcpp" and a global 
setting override it
  >
  >   Regarding other comments:
  > - I guess you mean if (mDefault == mReference) { with if (value == 
"defaultcpp") in that case mDefault is equal to either default from cpp value 
or default from global file.
  > - Your first assertion is right.
  >
  >   Still need to fix documentation
  
  
  Indeed there is still some question regarding if (value == "defaultcpp")
  kwindowconfig fix seems wrong

REPOSITORY
  R237 KConfig

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

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Benjamin Port]

bport updated this revision to Diff 82028.
bport added a comment.


  - Fix comments
  - Ensure we don't have problem if we set value to "defaultcpp" and a global 
setting override it
  
  Regarding other comments:
  
  - I guess you mean if (mDefault == mReference) { with if (value == 
"defaultcpp")
  
  in that case mDefault is equal to either default from cpp value or default 
from global file.
  
  - Your first assertion is right.
  
  Still need to fix documentation

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28221?vs=80624=82028

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

AFFECTED FILES
  autotests/kconfigskeletontest.cpp
  autotests/kconfigskeletontest.h
  autotests/kconfigtest.cpp
  autotests/kconfigtest.h
  src/core/kconfigdata.cpp
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/gui/kwindowconfig.cpp

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29455: KNS: Deprecate isRemote method and handle parse error properly

[Alexander Lohnau]

alex updated this revision to Diff 82024.
alex retitled this revision from "KNS: Remove isRemote method and handle parse 
error properly" to "KNS: Deprecate isRemote method and handle parse error 
properly".
alex added a comment.


  Make isRemote deprecated

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29455?vs=82019=82024

BRANCH
  fix_isremote_stuff (branched from master)

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/engine.cpp
  src/core/installation.cpp
  src/core/installation.h

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29454: [TaglibExtractor] Add support for Audible "Enhanced Audio" audio books

[Nathaniel Graham]

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


  Nice!

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

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

To: bruns, #frameworks, #baloo, ngraham, astippich, mgallien
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29455: KNS: Remove isRemote method and handle parse error properly

[Dan Leinir Turthra Jensen]

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

INLINE COMMENTS

> installation.h:76
>  bool readConfig(const KConfigGroup );
> -bool isRemote() const;
>  

i'm afraid this is an exported class, at most we can mark this as deprecated 
(with a bit of explanation), we can't remove it...

REPOSITORY
  R304 KNewStuff

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

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29447: Fix showing updates when the option is selected

[Alexander Lohnau]

alex added a comment.


  No problem, always happy to help 

REPOSITORY
  R304 KNewStuff

BRANCH
  fix-show-only-updates (branched from master)

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

To: leinir, #knewstuff, #plasma, bugseforuns, ngraham, #frameworks
Cc: alex, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, michaelh, ZrenBot, ngraham, bruns, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D29447: Fix showing updates when the option is selected

[Dan Leinir Turthra Jensen]

leinir marked 2 inline comments as done.
leinir added a comment.


  Thanks for making me realise that it doesn't have to be quite so elaborate, 
@alex ;)

REPOSITORY
  R304 KNewStuff

BRANCH
  fix-show-only-updates (branched from master)

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

To: leinir, #knewstuff, #plasma, bugseforuns, ngraham, #frameworks
Cc: alex, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, michaelh, ZrenBot, ngraham, bruns, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D29447: Fix showing updates when the option is selected

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 82021.
leinir added a comment.


  As @alex suggests, just use qlist::contains, it is supposed to be
  reasonably cheap, so... yup, trust the framework! ;)
  
  - Just use qlist::contains

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29447?vs=82003=82021

BRANCH
  fix-show-only-updates (branched from master)

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

AFFECTED FILES
  src/core/itemsmodel.cpp
  src/qtquick/qml/Page.qml
  src/qtquick/quickitemsmodel.cpp

To: leinir, #knewstuff, #plasma, bugseforuns, ngraham, #frameworks
Cc: alex, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, michaelh, ZrenBot, ngraham, bruns, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D29455: KNS: Remove isRemote method and handle parse error properly

[Alexander Lohnau]

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

REVISION SUMMARY
  The isRemote check and the FIXMEs are removed, instead the result from the 
empty target check is properly handled.
  
  The same check as in isRemote is done in the Installation::readConfig method 
(line 122).
  If this fails a warning is given and in the engine is an error emitted. This 
error gets shown in the GUI.
  Then there are no other checks needed.

TEST PLAN
  Delete the line TargetDir from the dolphin servicemenu knsrc file.
  (Dolphin repository src/settings/services/servicemenu.knsrc).
  
  Before:
  F8256199: Screenshot_20200423_202404.png 

  After, an error message is shown:
  F8256197: Screenshot_20200423_202239.png 


REPOSITORY
  R304 KNewStuff

BRANCH
  fix_isremote_stuff (branched from master)

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

AFFECTED FILES
  src/core/engine.cpp
  src/core/installation.cpp
  src/core/installation.h

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29447: Fix showing updates when the option is selected

[Dan Leinir Turthra Jensen]

leinir added inline comments.

INLINE COMMENTS

> alex wrote in itemsmodel.cpp:71
> On second thought why not just use:
> `bool duplicate =  m_entries.contains(entry);`

Hmm... i do wonder slight of the cost of that, but also... much simpler code, 
so... basically can just go

  if (!m_entries.contains(entry)) {

below, rather than this more elaborate version.

> alex wrote in itemsmodel.cpp:71
> Use qAsConst(m_entries) and space before & not after :-)

Quite, thank you :)

REPOSITORY
  R304 KNewStuff

BRANCH
  fix-show-only-updates (branched from master)

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

To: leinir, #knewstuff, #plasma, bugseforuns, ngraham, #frameworks
Cc: alex, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, michaelh, ZrenBot, ngraham, bruns, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D29454: [TaglibExtractor] Add support for Audible "Enhanced Audio" audio books

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Frameworks, Baloo, ngraham, astippich, mgallien.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  Audible AAX files are standard ISO Base Media File Format (ISOBMFF) files,
  often called "MP4" or "Quicktime". Metadata can thus be extracted with
  the taglib "MP4" code path.
  
  The test files have been created with ffmpeg and then edited with okteta/
  exiftool to resemble real AAX files, which can not be included for
  copyright reasons.
  
  https://gitlab.freedesktop.org/xdg/shared-mime-info/-/merge_requests/76
  is required to correctly detect AAX files.

TEST PLAN
  ctest

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

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

AFFECTED FILES
  autotests/samplefiles/no-meta/test.aax
  autotests/samplefiles/nocoverage_test.aax
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.json

To: bruns, #frameworks, #baloo, ngraham, astippich, mgallien
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D29447: Fix showing updates when the option is selected

[Alexander Lohnau]

alex added inline comments.

INLINE COMMENTS

> itemsmodel.cpp:71
> +bool duplicate{false};
> +for (const EntryInternal  : qAsConst(m_entries)) {
> +if (existingEntry == entry) {

On second thought why not just use:
`bool duplicate =  m_entries.contains(entry);`

REPOSITORY
  R304 KNewStuff

BRANCH
  fix-show-only-updates (branched from master)

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

To: leinir, #knewstuff, #plasma, bugseforuns, ngraham, #frameworks
Cc: alex, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, michaelh, ZrenBot, ngraham, bruns, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D29445: [KOpenWithDialog] When pointing at a non-executable file print more meaningful error

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in kopenwithdialog.cpp:1006
> Right, hence the isEmpty() here. This check passes if 1) it doesn't exist, or 
> 2) it's not executable.

I was wondering how the QString returned by 
KIO::DesktopExecParser::executablePath() would work if it returned /usr/bin/foo 
(as it doesn't strip the path), so how does findExecutable() work in that case? 
... so I tested and it turns out, findExecutable() will work with:

- foo, if it can find it in $PATH and it's executable
- /usr/bin/foo, if it's executable (though I have to wonder how that qualifies 
as "executableName"?)
- /some/path/foo, as long as "foo" is executable, the docs don't say anything 
about this behaviour, however the implementation does indeed support this

This change covers the use case of an absolute path to a file that _exists_ but 
isn't _executable_.

And again, findExecutable() would be more useful if it reported some sort of 
error saying "I found it, but it's not executable".

> kopenwithdialog.cpp:1008
> +// Maybe it exists but isn't executable
> +if (QDir::isAbsolutePath(binaryName)) {
> +QFileInfo fi(binaryName);

Why not QFileInfo::isAbsolute()?

REPOSITORY
  R241 KIO

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

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

D29447: Fix showing updates when the option is selected

[Nathaniel Graham]

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

REPOSITORY
  R304 KNewStuff

BRANCH
  fix-show-only-updates (branched from master)

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

To: leinir, #knewstuff, #plasma, bugseforuns, ngraham, #frameworks
Cc: alex, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, michaelh, ZrenBot, ngraham, bruns, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D29451: KNS: Do not mark entry as installed if install script failed

[Alexander Lohnau]

alex edited the summary of this revision.
alex edited the test plan for this revision.

REPOSITORY
  R304 KNewStuff

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

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29451: KNS: Do not mark entry as installed if install script failed

[Alexander Lohnau]

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

REVISION SUMMARY
  If the install script failed/was aborted the entry
  gets marked as installed. Now the exit code is checked
  and a error message is displayed.

TEST PLAN
  Make sure to have D29119 . Install the 
deb package of the "Jetbrains" dolphin addon.
  When the authorization popup comes press escape. The dolphin installer
  shows an error popup and the entry is not marked as installed.
  If you are not on a debian based system it should throw an error anyway ;-).

REPOSITORY
  R304 KNewStuff

BRANCH
  handle_install_script_error (branched from master)

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

AFFECTED FILES
  src/core/installation.cpp

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29406: Add X-KDE-Original-Executable to Applications properties

[Méven Car]

meven abandoned this revision.
meven added a comment.


  This not necessary, spectacle should have its executable in its Exec desktop 
entry.

REPOSITORY
  R309 KService

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

To: meven, #frameworks, davidedmundson, apol, bport
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29447: Fix showing updates when the option is selected

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 82003.
leinir added a comment.


  Address comment by @alex
  
  - Fix style (and consty things)

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29447?vs=81993=82003

BRANCH
  fix-show-only-updates (branched from master)

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

AFFECTED FILES
  src/core/itemsmodel.cpp
  src/qtquick/qml/Page.qml
  src/qtquick/quickitemsmodel.cpp

To: leinir, #knewstuff, #plasma, bugseforuns, ngraham, #frameworks
Cc: alex, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, michaelh, ZrenBot, ngraham, bruns, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D25339: KateRenderer: Use representitive character in CJK to estimate the fontHeight.

[Sven Brauch]

brauch added a comment.


  In D25339#663915 , @fakefred wrote:
  
  > I second the as-an-option proposal. Hey, why not automatically increase the 
line height when CJK characters are detected?
  
  
  This issue has been around for years and actually yeah, I think that is the 
only viable solution unless somebody dives in and reworks the renderer to 
support variable line heights.

REPOSITORY
  R39 KTextEditor

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

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D29415: Add holiday file for DE-BE (Germany/Berlin)

[Volker Krause]

This revision was automatically updated to reflect the committed changes.
Closed by commit R175:c39d1eb12217: Add holiday file for DE-BE (Germany/Berlin) 
(authored by vkrause).

REPOSITORY
  R175 KHolidays

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29415?vs=81905=81999

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

AFFECTED FILES
  holidays/holidays.qrc
  holidays/plan2/holiday_de-be_de

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

D29050: WIP KRunner fix prepare/teardown signals

[Alexander Lohnau]

alex added a comment.


  How should I proceed with this patch?

REPOSITORY
  R308 KRunner

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

To: alex, meven, ngraham, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29447: Fix showing updates when the option is selected

[Alexander Lohnau]

alex added inline comments.

INLINE COMMENTS

> itemsmodel.cpp:71
> +bool duplicate{false};
> +for (const EntryInternal& existingEntry : m_entries) {
> +if (existingEntry == entry) {

Use qAsConst(m_entries) and space before & not after :-)

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #plasma, bugseforuns, ngraham, #frameworks
Cc: alex, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, michaelh, ZrenBot, ngraham, bruns, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D29447: Fix showing updates when the option is selected

[Dan Leinir Turthra Jensen]

leinir added reviewers: KNewStuff, Plasma, bugseforuns, ngraham, Frameworks.
leinir added projects: Plasma, KNewStuff.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #plasma, bugseforuns, ngraham, #frameworks
Cc: kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, ragreen, michaelh, ZrenBot, ngraham, bruns, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29447: Fix showing updates when the option is selected

[Dan Leinir Turthra Jensen]

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

REVISION SUMMARY
  This patch primarily fixes the issue that when picking the "Updateable Only" 
option on NewStuff.Page, we would be shown all the entries anyway. It also 
fixes duplicated entries on the "Installed Only" option, and further adds a 
little placeholder for when the view is expectedly empty.
  
  - Use the right connections for update entry display
  - While potentially expensive, only add entries to the model once
  - Use Kirigami.PlaceholderMessage to show we have no entries when we don't
  
  BUG:416762

TEST PLAN
  Without this patch, behaviour is as shown in 
https://bugs.kde.org/show_bug.cgi?id=416762
  
  With this patch, the behaviour is as expected (shows only updateable entries 
with that option selected, only shows installed entries once with that option, 
and shows a useful message when the list is empty and expected to be empty)

REPOSITORY
  R304 KNewStuff

BRANCH
  fix-show-only-updates (branched from master)

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

AFFECTED FILES
  src/core/itemsmodel.cpp
  src/qtquick/qml/Page.qml
  src/qtquick/quickitemsmodel.cpp

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

D29445: [KOpenWithDialog] When pointing at a non-executable file print more meaningful error

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> apol wrote in kopenwithdialog.cpp:1006
> This patch D29170  suggests that 
> findExecutable won't find non-executables.
> 
> Something's wrong.

Right, hence the isEmpty() here. This check passes if 1) it doesn't exist, or 
2) it's not executable.

REPOSITORY
  R241 KIO

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

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

D29381: Thumbnail text: use libmagic to detect encoding

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

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

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

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

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

REPOSITORY
  R320 KIO Extras

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

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

D29445: [KOpenWithDialog] When pointing at a non-executable file print more meaningful error

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> kopenwithdialog.cpp:1006
>  // Ensure that the typed binary name actually exists (#81190)
>  if (QStandardPaths::findExecutable(binaryName).isEmpty()) {
> +// Maybe it exists but isn't executable

This patch D29170  suggests that 
findExecutable won't find non-executables.

Something's wrong.

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29445: [KOpenWithDialog] When pointing at a non-executable file print more meaningful error

[Kai Uwe Broulik]

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

REVISION SUMMARY
  `QStandardPaths::findExecutable` only finds executable binaries, so when 
explicitly pointing it at an existing file, it would complain about it not 
existing, rather than telling you it's not executable.

TEST PLAN
  - Tried to associate a non-executable shell script with a file: it now told 
me what was up
  
  Before it would tell me file doesn't exist

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/widgets/kopenwithdialog.cpp

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

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

meven added a comment.


  In D29397#663902 , @broulik wrote:
  
  > > At the risk of asking a stupid question, why?
  >
  > The text thumbnailer should be able to produce readable text on high dpi, 
or the folder thumbnailer should render the folder icon sharply and the picture 
frames non-pixelated
  
  
  And this also makes the job of Applications easier, not having to make `width 
* devicePixelRatio, height * devicePixelRatio` everywhere but a single 
`setDevicePixelRatio` similar to Qt own functions.

REPOSITORY
  R241 KIO

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

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

D25339: KateRenderer: Use representitive character in CJK to estimate the fontHeight.

[Frederick Yin]

fakefred added a comment.


  I second the as-an-option proposal. Hey, why not automatically increase the 
line height when CJK characters are detected?

REPOSITORY
  R39 KTextEditor

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

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: sars, pshinjo, rjvbb, fakefred, anthonyfieroni, kde-frameworks-devel, 
kwrite-devel, rrosch, LeGast00n, cblack, domson, michaelh, ngraham, bruns, 
demsking, cullmann, dhaumann

D29397: KPreviewJob : Support for DeviceRatioPixel

[Kai Uwe Broulik]

broulik added a comment.


  > At the risk of asking a stupid question, why?
  
  The text thumbnailer should be able to produce readable text on high dpi, or 
the folder thumbnailer should render the folder icon sharply and the picture 
frames non-pixelated

REPOSITORY
  R241 KIO

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

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

D29397: KPreviewJob : Support for DeviceRatioPixel

[David Edmundson]

davidedmundson added a comment.


  > Allow users of KPreviewJob to request a devicePixelRatio for generated 
thumbnails.
  
  At the risk of asking a stupid question, why?
  
  As opposed to just having a width and height always be in device pixels. 
We're always working with pixmaps is there a reason to do anything with logical 
sizes?
  That's how I thought the current design worked.

REPOSITORY
  R241 KIO

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

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

D25339: KateRenderer: Use representitive character in CJK to estimate the fontHeight.

[Shinjo Park]

pshinjo added a comment.


  In D25339#663833 , @rjvbb wrote:
  
  > doesn't it give you US-ASCII canonical representations of every possible 
glyph?
  
  
  Whether it is possible or not, no CJK users will write TeX document using 
US-ASCII representation for all glyphs instead of their native alphabet, 
especially when the document is in their mother tongue. Welcome to the world of 
transliteration problems.

REPOSITORY
  R39 KTextEditor

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

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: sars, pshinjo, rjvbb, fakefred, anthonyfieroni, kde-frameworks-devel, 
kwrite-devel, rrosch, LeGast00n, cblack, domson, michaelh, ngraham, bruns, 
demsking, cullmann, dhaumann

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

meven updated this revision to Diff 81982.
meven added a comment.


  Improve doc, fix @since

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29397?vs=81971=81982

BRANCH
  preview-dpr

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

AFFECTED FILES
  src/widgets/previewjob.cpp
  src/widgets/previewjob.h
  src/widgets/thumbcreator.cpp
  src/widgets/thumbcreator.h

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

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

meven marked an inline comment as done.

REPOSITORY
  R241 KIO

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

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

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> previewjob.cpp:433
> +{
> +d_func()->devicePixelRatio = dpr;
> +}

Maybe make this static, so that apps have to do it once per app sort of like we 
do with `Qt::AA_UseHighDpiPixmaps`, rather than by KPreviewJob.

REPOSITORY
  R241 KIO

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

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

D29231: Add keyboard_shortcuts_inhibit protocol

[Benjamin Port]

bport abandoned this revision.
bport added a comment.


  Follow up on gitlab 
  https://invent.kde.org/kde/kwayland-server/-/merge_requests/1

REPOSITORY
  R127 KWayland

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

To: bport, zzag, davidedmundson, apol
Cc: romangg, crossi, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D25339: KateRenderer: Use representitive character in CJK to estimate the fontHeight.

[René J . V . Bertin]

rjvbb added a comment.


  >   It's "KTextEditor", not "KCodeEditor". 
  
  Yes, but look at the traditional meaning of a text editor, which typically 
means "plain text" editor. KTextEditor's design decision to use a single 
lineheight puts it squarely in that domain - to reply in style: `It's 
"TKextEditor", not "KRichTextEditor" (and even less "KWordProcessor")` ...
  
  > write CJK LaTeX articles in Kile
  
  Like it or not, TeX is code (or a universal language, depending on how you 
look at it). That's intentional (WYSIWIG vs. WYSIAYG and all that). And 
frankly, TeX code is the last place where I'd expect this kind of rendering 
problem: doesn't it give you US-ASCII canonical representations of every 
possible glyph? (I know that doesn't help manuscript readability, but hey, 
that's the choice you make :) ) One could also make the argument that maybe 
Kile should look for another rendering engine if they want to support something 
more advanced than KTextEditor can offer. There are Qt  classes that have "rich 
text" support which might be more appropriate (if they offer editing support), 
and on the other end of the scale there's the Calligra project which probably 
has the required libraries.
  
  Anyway, serving a worldwide userbase isn't a new goal, but also shouldn't 
IMHO drag down usability to some lowest common denominator IMHO. Hence my firm 
request to make any chance that does have that effect optional, one way or 
another - I do agree with @sars .

REPOSITORY
  R39 KTextEditor

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

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: sars, pshinjo, rjvbb, fakefred, anthonyfieroni, kde-frameworks-devel, 
kwrite-devel, rrosch, LeGast00n, cblack, domson, michaelh, ngraham, bruns, 
demsking, cullmann, dhaumann

D29406: Add X-KDE-Original-Executable to Applications properties

[Méven Car]

meven added a comment.


  In D29406#663112 , @apol wrote:
  
  > > If I understand correctly, it is necessary to use dbus spectacle in this 
case, so that dbus can manage the application instance and make sure we end up 
with only one, whether we launch the app, use a shortcut or launch from command 
line.
  >
  > I don't think that's a good reason. We have lots of applications that do 
that without having to be launched by dbus: plasmashell, krunner, yakuake, etc. 
It's done by using KDBusService.
  
  
  I didn't know about this, so did perhaps @davidedmundson in 
https://phabricator.kde.org/D19153#417025
  I guess spectacle should use `KDBusService(KDBusService::Unique` but it has 
always been in multiple instance since it was Dbus enabled 
09cd11881d828da35c46c48da79f2d988e6a78cc 
 and 
this require some refactoring unfortunately.

REPOSITORY
  R309 KService

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

To: meven, #frameworks, davidedmundson, apol, bport
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[David Faure]

dfaure added a comment.


  Oh, I thought it was sent as an int. But 8 is 
QImage::Format_ARGB8565_Premultiplied. Did you mean 0x80?

INLINE COMMENTS

> thumbcreator.h:191
> + * @class ThumbCreatorV3 thumbcreator.h 
> + * @since 5.70
> + */

5.70 is tagged already

REPOSITORY
  R241 KIO

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

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

D29420: Generate DBus interface

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> notifybypopup.cpp:115
>  
> - bool connected = QDBusConnection::sessionBus().connect(QString(), // 
> from any service
> -   
> QString::fromLatin1(dbusPath),

Previously it would accept the signal from any service which I find odd, 
though. Could you maybe check git logs to see if there was a reason for this?
It should survive restarts anyway and the service name is defined, so I really 
don't see why it used to be a blind connect.

> notifybypopup.cpp:364
> +QDBusPendingReply reply = *watcher;
> +notifications.insert(reply.argumentAt<0>(), notification);
> +});

You have `watcher` (which is parented to `notification`) and `notification` as 
contexts, but in the lambda you also access `this`. This looks dangerous. How 
about using `this` instead of `notification` as context object?

> notifybypopup.cpp:376
> +
> +QDBusPendingCall call = dbusInterface.GetCapabilities();
> +

This is `QDBusPendingReply` (or just `auto`...)

> notifybypopup.cpp:385
> +popupServerCapabilities = capabilities;
> +dbusServiceCapCacheDirty = false;
> +

Unrelated: I was wondering, do we actually monitor the notification service 
changing and make them dirty again?

> notifybypopup.cpp:388
> +// re-run notify() on all enqueued notifications
> +for (int i = 0, total = notificationQueue.size(); i < total; ++i) {
> +q->notify(notificationQueue.at(i).first, 
> notificationQueue.at(i).second);

range for?

REPOSITORY
  R289 KNotifications

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

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

D29381: Thumbnail text: use libmagic to detect encoding

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> sitter wrote in textcreator.cpp:38
> 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.

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.

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

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

meven updated this revision to Diff 81971.
meven marked 5 inline comments as done.
meven added a comment.


  Add Binary compatibility workarounds

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29397?vs=81836=81971

BRANCH
  preview-dpr

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

AFFECTED FILES
  src/widgets/previewjob.cpp
  src/widgets/previewjob.h
  src/widgets/thumbcreator.cpp
  src/widgets/thumbcreator.h

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

D29397: KPreviewJob : Support for DeviceRatioPixel

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> thumbcreator.h:139
>   */
> -virtual bool create(const QString , int width, int height, QImage 
> ) = 0; // KF6 TODO: turn first arg into QUrl (see 
> thumbnail/htmlcreator.cpp)
> +virtual bool create(const QString , int width, int height, QImage 
> , qreal devicePixelRatio = 1.0) = 0; // KF6 TODO: turn first arg into 
> QUrl (see thumbnail/htmlcreator.cpp)
>  

You can't do that

REPOSITORY
  R241 KIO

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

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

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

meven marked an inline comment as done.
meven added inline comments.

INLINE COMMENTS

> dfaure wrote in previewjob.cpp:717
> This here also breaks compatibility. Add a KF6 TODO to start the 
> serialization with a version number.
> 
> Meanwhile a hack is needed, like `if (iFormat & 0x1000) { iFormat &= 0xFFF; 
> str >> imgDevicePixelRatio; }`
> and of course setting that 0x1000 flag in the slaves that have been updated 
> to provide the pixelratio.

I am not sure the values you gave for the hack bitmask are correct, iFormat is 
quint8, so how come it contain 0x1000

> dfaure wrote in thumbcreator.h:183
> You need to do like we once did: define an interface that inherits from 
> ThumbCreator.
> Call it V3
> 
> In the job, use dynamic_cast to test whether the object provided by the 
> kioslave supports the V3 interface or not.

I guess you mean in the thumbnail kio regarding instantiating Thumbcreator.
Anyway it works locally.

REPOSITORY
  R241 KIO

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

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