D25339: update lineHeight if boundingRect indicates a larger value.

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> katerenderer.cpp:1192
> +// trigger view update, if any!
> +QMetaObject::invokeMethod(m_view, "updateRendererConfig", 
> Qt::QueuedConnection);
> +}

Can you use functor here, instead of string.

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

D25339: update lineHeight if boundingRect indicates a larger value.

[Xuetian Weng]

xuetianweng retitled this revision from "KateRenderer: Use representitive 
character in CJK to estimate the fontHeight." to "update lineHeight if 
boundingRect indicates a larger value.".
xuetianweng edited the summary of this revision.
xuetianweng edited the test plan for this revision.

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

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

[Xuetian Weng]

xuetianweng added a comment.


  I'm not sure if this is the right way to do it or it might cause any glitch,  
but here we go. Upon line rendering, update the maximum height we've ever seen.
  
  Though line height won't shrink during the edit phase, it will back to the 
actual value upon save.

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

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

[Xuetian Weng]

xuetianweng updated this revision to Diff 82170.
xuetianweng added a comment.


  Use actual line height instead of representitive character.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25339?vs=80607=82170

BRANCH
  arcpatch-D25339

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

AFFECTED FILES
  src/render/katerenderer.cpp
  src/render/katerenderer.h
  src/view/kateview.h

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

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

[Frederick Yin]

fakefred added a comment.


  In D25339#665351 , @xuetianweng 
wrote:
  
  > ... or you could give me some hint on that?
  
  
  Too bad I can't; it's been no more than a month since I began my study into 
KDE applications. I'd love to test out if someone provides a genius solution, 
though.

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

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

[Xuetian Weng]

xuetianweng 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?
  
  
  So the thing is we need to maintain a such height depending on the document, 
not so sure if that is even possible (or whether it could be costly) or you 
could give me some hint on that?
  Actually, I'd rather to calculate the height based on actual character in the 
document because during my test, mixing Arabic together to CJK and latin could 
still have problem (likely same for Cyrillic).

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

D29238: Use Standard "Show/Hide Hidden files" shortcuts in directory chooser dialog

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R135:f9f3f6ca260a: Use Standard Show/Hide Hidden 
files shortcuts in directory chooser dialog (authored by ngraham).

REPOSITORY
  R135 Integration for Qt applications in Plasma

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29238?vs=81378=82151

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

AFFECTED FILES
  CMakeLists.txt
  src/platformtheme/kdirselectdialog.cpp

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

D29238: Use Standard "Show/Hide Hidden files" shortcuts in directory chooser dialog

[Noah Davis]

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

REPOSITORY
  R135 Integration for Qt applications in Plasma

BRANCH
  use-standard-show-hide-files-shortcuts (branched from master)

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

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

D29479: Fix rounded borders

[Niccolò Venerandi]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:1bc004e5c65f: Fix rounded borders (authored by niccolove).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D29479?vs=82133=82147#toc

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29479?vs=82133=82147

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

AFFECTED FILES
  src/declarativeimports/plasmaextracomponents/qml/PlasmoidHeading.qml
  src/desktoptheme/breeze/widgets/plasmoidheading.svg

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

D29479: Fix rounded borders

[Kai Uwe Broulik]

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

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

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

D29479: Fix rounded borders

[Niccolò Venerandi]

niccolove marked 2 inline comments as done.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D29479: Fix rounded borders

[Niccolò Venerandi]

niccolove added inline comments.

INLINE COMMENTS

> broulik wrote in PlasmoidHeading.qml:74
> Where is `plasmoid` defined?

It's not necessary to define it, when you use this in a plasmoid it takes it 
from the context

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D29479: Fix rounded borders

[Niccolò Venerandi]

niccolove marked an inline comment as done.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D29479: Fix rounded borders

[Niccolò Venerandi]

niccolove updated this revision to Diff 82133.
niccolove added a comment.


  Round things up again

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29479?vs=82131=82133

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/plasmaextracomponents/qml/PlasmoidHeading.qml
  src/desktoptheme/breeze/widgets/plasmoidheading.svg

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

D29479: Fix rounded borders

[Niccolò Venerandi]

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


  In D29479#664767 , @broulik wrote:
  
  > Notifications don't use `PlasmoidHeading`
  
  
  Heh, it was actually an unrelated mistake: D29490 


REPOSITORY
  R242 Plasma Framework (Library)

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

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

D29479: Fix rounded borders

[Niccolò Venerandi]

niccolove updated this revision to Diff 82131.
niccolove added a comment.


  Address some feedback

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29479?vs=82078=82131

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/plasmaextracomponents/qml/PlasmoidHeading.qml
  src/desktoptheme/breeze/widgets/plasmoidheading.svg

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

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

[Nathaniel Graham]

ngraham added a comment.


  In D29485#664977 , @ahmadsamir 
wrote:
  
  > I couldn't seem to test the m_privilegeExecutionEnabled stuff, i.e. using 
dolphin, the paste action is disabled if the dir isn't owned by me.
  
  
  See D7563  (assistance would be 
appreciated if you have any idea how to make that work).

REPOSITORY
  R241 KIO

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

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

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

[Ahmad Samir]

ahmadsamir added a comment.


  I couldn't seem to test the m_privilegeExecutionEnabled stuff, i.e. using 
dolphin, the paste action is disabled if the dir isn't owned by me.

REPOSITORY
  R241 KIO

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

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

D29238: Use Standard "Show/Hide Hidden files" shortcuts in directory chooser dialog

[Nathaniel Graham]

ngraham added a comment.


  Ping.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

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

D29123: Do not mark entry as uninstalled if uninstallation script failed

[Alexander Lohnau]

alex updated this revision to Diff 82114.
alex added a comment.


  Ensure bic

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29123?vs=81918=82114

BRANCH
  arcpatch-D29123_1

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

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

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

D29123: Do not mark entry as uninstalled if uninstallation script failed

[Alexander Lohnau]

alex added inline comments.

INLINE COMMENTS

> leinir wrote in installation.h:124
> Hmm... i find myself wondering what effect this has on BIC... something tells 
> me it might not be so great... Specifically, 
> https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B#The_Do.27s_and_Don.27ts
>  says that you cannot change the signature of existing functions of any 
> type... However, it feels more like a problem with the documentation - the 
> idea is that the entries passed into functions aren't changed (and this 
> really should probably be a const reference, but for some reason it isn't, 
> and again we can't change that for bic-iness), and so really what /should/ 
> happen is that rather than saying "The entry instance will be updated" and so 
> on, what it should be doing is say "If the entry is successfully uninstalled, 
> listening to signalEntryChanged(const KNSCore::EntryInternal &) for an entry 
> equal to the one you have passed in will allow you to detect the result of 
> calling the function".
> 
> That's what it's already doing, and how it is used in places which call the 
> function, so probably makes sense to fix that.
> 
> In the longer term (think KF6), i would also quite like all the functionality 
> here to end up entirely asynchronous.

I told myself yesterday that I am going to have a look at this patch again^^
And you are absolutely right :-)

REPOSITORY
  R304 KNewStuff

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

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

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

[Ahmad Samir]

ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, dfaure, meven, sitter.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  Use KIO::FileSystemFreeSpaceJob to check free space for remote urls.
  
  Thanks to sitter for pin-pointing the responsible code in the bug report,
  and to kbroulik for pointing out the existence of KIO::FileSystemFreeSpaceJob.
  
  Also use UDSEntry to check writability for both local and remote urls.
  
  BUG: 418443
  
  FIXED-IN: 5.71

TEST PLAN
  testtrash unit test fails (again...)
  
  - Find/create a local partition with a small capacity, copying a file/dir 
that is bigger than the partition will show the error message as before
  - Do the same for a remote url (I tested with sftp://), an error message 
about not having enough free space is shown

REPOSITORY
  R241 KIO

BRANCH
  l-freespace-remote-2 (branched from master)

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

AFFECTED FILES
  src/core/copyjob.cpp

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

KDE CI: Frameworks » plasma-framework » kf5-qt5 FreeBSDQt5.14 - Build # 38 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/plasma-framework/job/kf5-qt5%20FreeBSDQt5.14/38/
 Project:
kf5-qt5 FreeBSDQt5.14
 Date of build:
Wed, 06 May 2020 14:56:08 +
 Build duration:
4 min 15 sec and counting
   JUnit Tests
  Name: projectroot Failed: 0 test(s), Passed: 11 test(s), Skipped: 0 test(s), Total: 11 test(s)

D29434: Use small font for ExpandableListItem subtitle

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:416f82f8f7f5: Use small font for ExpandableListItem 
subtitle (authored by ngraham).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29434?vs=81956=82110

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

AFFECTED FILES
  src/declarativeimports/plasmaextracomponents/qml/ExpandableListItem.qml

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

D29372: Taiwan: Use "zh_TW" language code

[Weisi Dai]

weisi abandoned this revision.
weisi added a comment.


  Closing this revision. Let's get D29223  
in.

REPOSITORY
  R175 KHolidays

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

To: weisi, winterz
Cc: nhiga, cgiboudeaux, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D29373: Taiwan: Hardcoding holidays based on the Lunar Calendar; Minor update to the holiday list

[Weisi Dai]

weisi abandoned this revision.
weisi added a comment.


  Closing this revision. Let's get D29223  
in.

REPOSITORY
  R175 KHolidays

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

To: weisi, winterz, cgiboudeaux
Cc: nhiga, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29483: [knewfilemenu] Show inline warning when creating items with leading or trailing spaces

[Nathaniel Graham]

ngraham updated this revision to Diff 82102.
ngraham added a comment.


  Spell "oddities" correctly

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29483?vs=82101=82102

BRANCH
  
show-warnings-for-leading-and-trailing-spaces-when-creating-files-or-folders-and-wow-this-is-a-long-branch-name-at-this-point
 (branched from master)

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

AFFECTED FILES
  src/filewidgets/knewfilemenu.cpp

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

D29483: [knewfilemenu] Show inline warning when creating items with leading or trailing spaces

[Nathaniel Graham]

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

REVISION SUMMARY
  This is a somewhat unusual thing for a user to do intentionally, so let's 
show a
  warning to make sure they're aware that they're doing it.
  
  BUG: 421075
  FIXED-IN: 5.71

TEST PLAN
  F8289470: Screenshot_20200506_081625.png 

  F8289471: Screenshot_20200506_081646.png 


REPOSITORY
  R241 KIO

BRANCH
  
show-warnings-for-leading-and-trailing-spaces-when-creating-files-or-folders-and-wow-this-is-a-long-branch-name-at-this-point
 (branched from master)

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

AFFECTED FILES
  src/filewidgets/knewfilemenu.cpp

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

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

[Alexander Lohnau]

alex updated this revision to Diff 82100.
alex added a comment.


  Make lambda inline, capture variables

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29451?vs=82004=82100

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

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

[Alexander Lohnau]

alex marked 2 inline comments as done.

REPOSITORY
  R304 KNewStuff

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

To: alex, #knewstuff, ngraham, leinir
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 82098.
alex marked an inline comment as done.
alex added a comment.


  Typo and error message
  
  If the error message is on two lines they aren't very readable, but
  that is an issue for another day :-).

REPOSITORY
  R304 KNewStuff

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

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

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

[Dan Leinir Turthra Jensen]

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


  Apart from these couple of details, it looks pretty good :) (i'd say just fix 
and commit, but one of them's a tiny bit larger than just a typo fix ;) )

INLINE COMMENTS

> engine.cpp:174
>  if (!m_installation->readConfig(group)) {
> +Q_EMIT signalError(i18n("Failed to read config file %1", 
> configfile));
>  return false;

"Failed to read config file" is more a specific issue with setting up the 
installation handler, so... "Could not initialise the installation handler for 
%1. This is a critical error and should be reported to the application author" 
would probably be a little clearer... It is /supposed/ to not happen, and is 
really quite critical, so the best we can reasonably do is direct the blame to 
the appropriate location.

> installation.h:78
> +#if KNEWSTUFFCORE_ENABLE_DEPRECATED_SINCE(5, 71)
> +KNEWSTUFFCORE_DEPRECATED_VERSION(5, 71, "No longer use, feature 
> obsulete")
>  bool isRemote() const;

Obsolete, not obsulete :) (just a nitpick, really :) )

REPOSITORY
  R304 KNewStuff

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

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

D29123: Do not mark entry as uninstalled if uninstallation script failed

[Dan Leinir Turthra Jensen]

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


  Sorry about missing that bic issue before...

INLINE COMMENTS

> leinir wrote in installation.cpp:653
> Unless you report the entry as changed, the cache will not be updated and the 
> entire reporting side will fall down. Please put that line back :)

In reference to the comment about bic issues above (and specifically how the 
function is /supposed/ to be interpreted), this is exactly the point where that 
confusion becomes perhaps a little more obvious - i hadn't noticed what you 
were doing before, expecting that entry instance to change, but as you can see, 
the signal there is important :)

> installation.h:124
>   */
> -void uninstall(KNSCore::EntryInternal entry);
>  

Hmm... i find myself wondering what effect this has on BIC... something tells 
me it might not be so great... Specifically, 
https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B#The_Do.27s_and_Don.27ts
 says that you cannot change the signature of existing functions of any type... 
However, it feels more like a problem with the documentation - the idea is that 
the entries passed into functions aren't changed (and this really should 
probably be a const reference, but for some reason it isn't, and again we can't 
change that for bic-iness), and so really what /should/ happen is that rather 
than saying "The entry instance will be updated" and so on, what it should be 
doing is say "If the entry is successfully uninstalled, listening to 
signalEntryChanged(const KNSCore::EntryInternal &) for an entry equal to the 
one you have passed in will allow you to detect the result of calling the 
function".

That's what it's already doing, and how it is used in places which call the 
function, so probably makes sense to fix that.

In the longer term (think KF6), i would also quite like all the functionality 
here to end up entirely asynchronous.

REPOSITORY
  R304 KNewStuff

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

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

D29479: Fix rounded borders

[Kai Uwe Broulik]

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


  Notifications don't use `PlasmoidHeading`

INLINE COMMENTS

> PlasmoidHeading.qml:71
> +enabledBorders: {
> +var borders = new Array()
> +borders |= PlasmaCore.FrameSvg.LeftBorder

Use `[]` instead of `new Array()`

> PlasmoidHeading.qml:72
> +var borders = new Array()
> +borders |= PlasmaCore.FrameSvg.LeftBorder
> +borders |= PlasmaCore.FrameSvg.RightBorder

How is operator `|=` supposed to work with an `Array`?!

> PlasmoidHeading.qml:74
> +borders |= PlasmaCore.FrameSvg.RightBorder
> +if (plasmoid.location !== PlasmaCore.Types.TopEdge || location 
> != PlasmoidHeading.Location.Header) {
> +borders |= PlasmaCore.FrameSvg.TopBorder

Where is `plasmoid` defined?

> plasmoidheading.svg:9
> +
> +
> +

You've just added a gazillion non-integer coordinates

REPOSITORY
  R242 Plasma Framework (Library)

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

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

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

[Dan Leinir Turthra Jensen]

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


  A bit nitpicky, that first one, the second's more serious (i'd like to avoid 
that in new code), but looks good otherwise :)

INLINE COMMENTS

> installation.cpp:355
>  QProcess* p = runPostInstallationCommand(installedFiles.size() 
> == 1 ? installedFiles.first() : targetPath);
> -connect(p, static_cast QProcess::ExitStatus)>(::finished), this, installationFinished);
> +auto finishedLambda = [=](int exitCode, QProcess::ExitStatus) {
> +if (exitCode) {

Not really any reason to store it in a variable so much... In the rest of the 
codebase, unless the same lambda is used in more than one location, it's 
defined inline in the connect statement. Also, i realise some of the code has 
the capture everything thing going on, but that's me being silly when i first 
learned how to use them - if possible, only capture the things you actually 
need to capture :)

REPOSITORY
  R304 KNewStuff

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

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

D29479: Fix rounded borders

[Nathaniel Graham]

ngraham added a comment.


  This fixes the square corners on tray popups for me, but not notifications.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

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

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in kopenwithdialog.cpp:1008
> Why not QFileInfo::isAbsolute()?

Forgot that also existed, will update

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

D29223: Update Taiwanese holidays

[N. Higa]

nhiga added a comment.


  Officially-recognized days (including 言論自由日) and the dates for 2020 can be 
found in this document 
.
 Names without ※ means it is officially-recognized but is not stated in the law.
  
  There are many (>60) commemorative days (days without ※) so I do not plan to 
include them in the file (for the time being).

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

To: nhiga, winterz, cgiboudeaux, shrapnel
Cc: weisi, #kde_pim, kde-frameworks-devel, shrapnel, LeGast00n, cblack, 
fbampaloukas, dcaliste, michaelh, ngraham, bruns, dvasin, rodsevich, winterz, 
vkrause, mlaurent, knauss, dvratil

D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis added a comment.


  OK, I can agree with contributing upstream. I'll try talking on that bug 
report.

REPOSITORY
  R265 KConfigWidgets

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

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

D29223: Update Taiwanese holidays

[N. Higa]

nhiga updated this revision to Diff 82085.
nhiga added a comment.


  Added 植樹節.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29223?vs=81833=82085

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

AFFECTED FILES
  holidays/holidays.qrc
  holidays/plan2/holiday_tw_zh
  holidays/plan2/holiday_tw_zh-tw

To: nhiga, winterz, cgiboudeaux, shrapnel
Cc: weisi, #kde_pim, kde-frameworks-devel, shrapnel, LeGast00n, cblack, 
fbampaloukas, dcaliste, michaelh, ngraham, bruns, dvasin, rodsevich, winterz, 
vkrause, mlaurent, knauss, dvratil

D25814: [KColorScheme] Add SeparatorColor

[David Edmundson]

davidedmundson added a comment.


  Relevant link on that last comment: 
https://bugreports.qt.io/browse/QTBUG-63331
  
  They actively seeked our opinion on colour roles

REPOSITORY
  R265 KConfigWidgets

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

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

D25814: [KColorScheme] Add SeparatorColor

[Christoph Feck]

cfeck added a comment.


  There is QPalette::Button, but I don't see any hover/focus colors in QPalette.
  
  If we want more roles, we seriously need to contribute them upstream. Qt 6 is 
a chance to avoid diverging more.

REPOSITORY
  R265 KConfigWidgets

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

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

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

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


  Fix bitmask check

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29397?vs=82071=82079

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: davidedmundson, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29479: Fix rounded borders

[Niccolò Venerandi]

niccolove edited the test plan for this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29479: Fix rounded borders

[Niccolò Venerandi]

niccolove edited the test plan for this revision.
niccolove added a reviewer: Plasma.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29479: Fix rounded borders

[Niccolò Venerandi]

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

REVISION SUMMARY
  This makes borders of elements using plasmoidHeading rounded again

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/plasmaextracomponents/qml/PlasmoidHeading.qml
  src/desktoptheme/breeze/widgets/plasmoidheading.svg

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

D29223: Update Taiwanese holidays

[Christophe Giboudeaux]

cgiboudeaux added a comment.


  If everything is sorted and both of you agree, please accept one of the 
reviews and we'll happily push it if necessary.

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

To: nhiga, winterz, cgiboudeaux, shrapnel
Cc: weisi, #kde_pim, kde-frameworks-devel, shrapnel, LeGast00n, cblack, 
fbampaloukas, dcaliste, michaelh, ngraham, bruns, dvasin, rodsevich, winterz, 
vkrause, mlaurent, knauss, dvratil

D29461: Fix kio-extras build on Windows

[Hannah von Reth]

vonreth added inline comments.

INLINE COMMENTS

> brute4s99 wrote in CMakeLists.txt:26
> I just built libssh 0.9.4, and kio-extras builds fine without this line 
> change. Maybe we should try getting latest libssh onboard. Can I help in that 
> somehow @vonreth ?

yes pls update libssh

REPOSITORY
  R320 KIO Extras

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

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

D29461: Fix kio-extras build on Windows

[Piyush Aggarwal]

brute4s99 added inline comments.

INLINE COMMENTS

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

I just built libssh 0.9.4, and kio-extras builds fine without this line change. 
Maybe we should try getting latest libssh onboard. Can I help in that somehow 
@vonreth ?

REPOSITORY
  R320 KIO Extras

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

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

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

meven planned changes to this revision.
meven added a comment.


  In D29397#664605 , @dfaure wrote:
  
  > In D29397#664536 , @meven wrote:
  >
  > > In D29397#663800 , @dfaure 
wrote:
  > >
  > > > Oh, I thought it was sent as an int. But 8 is 
QImage::Format_ARGB8565_Premultiplied. Did you mean 0x80?
  > >
  > >
  > > No I meant 8, since format is passed as quint8 (a single byte, 8-bit) and 
the format is forced to QImage::Format_ARGB32 "5", so this works.
  >
  >
  > OK, this works today. But if one day we want to start actually using other 
image formats, we'll end up with a clash here.
  >  Why not use 0x80 in order to stay away from valid image format values? 
This seems safer to me, in the long run.
  
  
  I did wrong calculation about how big a 8-bit integer is, believing 0x8 was 
the most significant bit, while you correct me here it is 0x80, thanks.
  Will update accordingly.

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

D29461: Fix kio-extras build on Windows

[Harald Sitter]

sitter added a comment.


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

INLINE COMMENTS

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

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

REPOSITORY
  R320 KIO Extras

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

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

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

meven updated this revision to Diff 82071.
meven marked an inline comment as done.
meven added a comment.


  Add @since to setDefaultDevicePixelRatio

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29397?vs=82056=82071

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: davidedmundson, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D25814: [KColorScheme] Add SeparatorColor

[Arjen Hiemstra]

ahiemstra added a comment.


  I actually ran into this with the new Card designs I implemented in Kirigami. 
Kirigami right now does a lot of `tint(backgroundColor, textColor)` to generate 
separator colours. However, this leads to potential differences when the color 
set is View vs. Window or other cases, making separators looks slightly 
different based on where they are used. I don't think that's a good idea 
personally and in any case, having an explicit separator color would make it an 
explicit design decision instead of an implicit technical artifact. So +1 from 
my side for adding this.

REPOSITORY
  R265 KConfigWidgets

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

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

D29461: Fix kio-extras build on Windows

[Piyush Aggarwal]

brute4s99 added a subscriber: sitter.
brute4s99 added a comment.


  @sitter could you please review this change as well?

REPOSITORY
  R320 KIO Extras

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

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

D29461: Fix kio-extras build on Windows

[Piyush Aggarwal]

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


  updated the diff

REPOSITORY
  R320 KIO Extras

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

BRANCH
  arcpatch-D29461

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

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

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

D29397: KPreviewJob : Support for DeviceRatioPixel

[David Faure]

dfaure added a comment.


  In D29397#664536 , @meven wrote:
  
  > In D29397#663800 , @dfaure wrote:
  >
  > > Oh, I thought it was sent as an int. But 8 is 
QImage::Format_ARGB8565_Premultiplied. Did you mean 0x80?
  >
  >
  > No I meant 8, since format is passed as quint8 (a single byte, 8-bit) and 
the format is forced to QImage::Format_ARGB32 "5", so this works.
  
  
  OK, this works today. But if one day we want to start actually using other 
image formats, we'll end up with a clash here.
  Why not use 0x80 in order to stay away from valid image format values? This 
seems safer to me, in the long run.

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

D29447: Fix showing updates when the option is selected

[Dan Leinir Turthra Jensen]

This revision was automatically updated to reflect the committed changes.
Closed by commit R304:25d391c6f36b: Fix showing updates when the option is 
selected (authored by leinir).

REPOSITORY
  R304 KNewStuff

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

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

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

[David Faure]

dfaure updated this revision to Diff 82058.
dfaure added a comment.


  -void KIO::OpenUrlJob::setRunFlags(KIO::ApplicationLauncherJob::RunFlags 
runFlags)
  +void KIO::OpenUrlJob::setDeleteTemporaryFile(bool b)
  
  The more I think about it, the least I like the use of flags here.
  
  1. they are from the wrong class as Kai-Uwe pointed out, but more importantly:
  2. the other bool setters here are for unrelated concerns, better keep them 
separate.
  
  Are we doing lineEdit->setDragEnabled(true); 
lineEdit->setClearButtonEnabled(true); lineEdit->setReadOnly(true);
  or are we doing lineEdit->setFlags(QLineEdit::DragEnabled | 
QLineEdit::ClearButtonEnabled | QLineEdit::ReadOnly)?

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29385?vs=81840=82058

BRANCH
  2020_05_openurljob

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/krununittest.cpp
  autotests/openurljobtest.cpp
  autotests/openurljobtest.h
  src/core/kurlauthorized.h
  src/gui/CMakeLists.txt
  src/gui/applicationlauncherjob.cpp
  src/gui/applicationlauncherjob.h
  src/gui/openurljob.cpp
  src/gui/openurljob.h
  src/gui/openurljobhandlerinterface.cpp
  src/gui/openurljobhandlerinterface.h
  src/widgets/CMakeLists.txt
  src/widgets/jobuidelegate.cpp
  src/widgets/kdesktopfileactions.cpp
  src/widgets/kopenwithdialog.cpp
  src/widgets/kopenwithdialog.h
  src/widgets/krun.cpp
  src/widgets/widgetsopenurljobhandler.cpp
  src/widgets/widgetsopenurljobhandler.h

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

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

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


  I have pretty much the patch in kio-extras ready.
  So I am seeking

INLINE COMMENTS

> broulik wrote in previewjob.cpp:433
> Not a fan. You can have different dpi per screen.
> Maybe instead we should have a `QWindow*` method or constructor to take dpr 
> from

> Not a fan. You can have different dpi per screen.

What you imply is incorrect.
In Wayland, QGuiApplication::devicePixelRatio always returns the maximum scale 
of any screens (and ceiled up, so my 1.25 scale is returned as 2), and always 
the same value regardless on the scale of the screen where the app is. Only at 
painting does the scale makes a difference per screen.

In X, well there is only one scale anyway.

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

[Méven Car]

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


  Add a static setDefaultDevicePixelRatio method to PreviewJob

REPOSITORY
  R241 KIO

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

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: davidedmundson, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> meven wrote in previewjob.cpp:433
> 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.

Not a fan. You can have different dpi per screen.
Maybe instead we should have a `QWindow*` method or constructor to take dpr from

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

[Méven Car]

meven added a comment.


  In D29397#663800 , @dfaure wrote:
  
  > Oh, I thought it was sent as an int. But 8 is 
QImage::Format_ARGB8565_Premultiplied. Did you mean 0x80?
  
  
  No I meant 8, since format is passed as quint8 (a single byte, 8-bit) and the 
format is forced to QImage::Format_ARGB32 "5", so this works.
  From kio-extra thumbnails :
  
if( img.format() != QImage::Format_ARGB32 ) { // KIO::PreviewJob and this 
code below completely ignores colortable :-/,
img = img.convertToFormat(QImage::Format_ARGB32); //  so make sure 
there is none
}
// Keep in sync with kio/src/previewjob.cpp

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

D29381: Thumbnail text: use libmagic to detect encoding

[Méven Car]

meven added a comment.


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.
  
  Well I am doubting now libmagic is the way to go:
  It is quite limited in encoding detection :
  https://invent.kde.org/snippets/875
  Compared to 
https://en.wikipedia.org/wiki/Character_encoding#Common_character_encodings

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

D29461: Fix kio-extras build on Windows

[Méven Car]

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


  We have :
  kio-extras/cmake/Findlibssh.cmake
  
  To do that. I don't think copy/pasting code to `sftp/CMakeLists.txt` is 
necessary.

REPOSITORY
  R320 KIO Extras

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

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