D25117: file ioslave: stop copying as soon as the ioslave is killed

[Méven Car]

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


  Add an env variable KIOSLAVE_FILE_ENABLE_TESTMODE to slow down copy file 
operations to make tests reliable

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25117?vs=74542&id=74543

BRANCH
  arcpatch-D25117

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

AFFECTED FILES
  autotests/jobtest.cpp
  src/core/filecopyjob.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

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

D26131: Braces around for, break early.

[Kevin Ottens]

ervin added a comment.


  In D26131#602149 , @dfaure wrote:
  
  > No, never const QChar&.
  >  QChar is a short int.
  
  
  Indeed.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin, dfaure
Cc: kossebau, ervin, patrickelectric, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25117: file ioslave: stop copying as soon as the ioslave is killed

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> jobtest.cpp:2077
> +} else {
> +QVERIFY(QFile::exists(destToCheck));
> +}

This is not right. It might hide a bug.

REPOSITORY
  R241 KIO

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

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

D25117: file ioslave: stop copying as soon as the ioslave is killed

[Méven Car]

meven planned changes to this revision.

REPOSITORY
  R241 KIO

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

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

D25117: file ioslave: stop copying as soon as the ioslave is killed

[Méven Car]

meven added a comment.


  In D25117#602227 , @meven wrote:
  
  > Fix test
  
  
  I was missing the case when the dest file existed before copying.
  I think this is ready for landing.

REPOSITORY
  R241 KIO

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

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

D25117: file ioslave: stop copying as soon as the ioslave is killed

[Méven Car]

meven updated this revision to Diff 74542.
meven marked 6 inline comments as done.
meven added a comment.


  Fix test

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25117?vs=72832&id=74542

BRANCH
  arcpatch-D25117

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

AFFECTED FILES
  autotests/jobtest.cpp
  src/core/filecopyjob.cpp
  src/ioslaves/file/file_unix.cpp

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

KDE CI: Frameworks » kdelibs4support » kf5-qt5 FreeBSDQt5.13 - Build # 57 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kdelibs4support/job/kf5-qt5%20FreeBSDQt5.13/57/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Wed, 29 Jan 2020 04:33:40 +
 Build duration:
5 min 25 sec and counting
   JUnit Tests
  Name: projectroot Failed: 2 test(s), Passed: 37 test(s), Skipped: 0 test(s), Total: 39 test(s)Failed: projectroot.autotests.kmimetypetestFailed: projectroot.autotests.kstandarddirstest

D26955: Fix background SVG colors and adjust background SVG corners

[Noah Davis]

ndavis abandoned this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

[Albert Astals Cid]

aacid added a comment.


  FWIW this broke artikulate since you changed the expected entry for engine: 
in KNS.ItemsModel

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: aacid, cfeck, davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, 
ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26131: Braces around for, break early.

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Getting rid of the inversion might be easier to read for simple humans (as me 
:) ):
  
const bool isAscii = std::all_of(std::begin(s), std::end(s), [](QChar a) { 
return a.unicode() <= 127; });

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin, dfaure
Cc: kossebau, ervin, patrickelectric, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D26868: Move newItem to private method in KConfigSourceGenerator

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D26868#602150 , @dfaure wrote:
  
  > There is indeed a QString overload for concatenating QLatin1String, but it 
will have to be converted char-by-char (from 8 bits to 16 bits), so isn't it 
faster to concatenate QStringLiterals?
  
  
  I have never seen numbers, but any time I asked someone who might have seen 
numbers, they told me: QStringLiteral only for final strings not further 
modified or given to operations. Sadly cannot find references now, but I asked 
often the recent years.
  Looking at the code, the `qt_from_latin1` helper method might be as efficient 
on most processors as the `memcpy` is perhaps, and making up for any other 
overhead the QStringLiteral-generated code has, perhaps atomic stuff around the 
generated QString instance is expensive compared to the copy logic?
  
  But ready to hear from real Qt experts once more, perhaps with numbers this 
time :)

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin, dfaure
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26868: Move newItem to private method in KConfigSourceGenerator

[David Faure]

dfaure added a comment.


  There is indeed a QString overload for concatenating QLatin1String, but it 
will have to be converted char-by-char (from 8 bits to 16 bits), so isn't it 
faster to concatenate QStringLiterals?

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin, dfaure
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26131: Braces around for, break early.

[David Faure]

dfaure added a comment.


  No, never const QChar&.
  QChar is a short int.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin, dfaure
Cc: ervin, patrickelectric, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26957: KateModeMenuList: don't overlap the scroll bar

[Nibaldo González]

nibags updated this revision to Diff 74531.
nibags added a comment.


  - Restore parameters

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26957?vs=74479&id=74531

BRANCH
  mode-menu-scrollbar-overlap

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

AFFECTED FILES
  src/mode/katemodemenulist.cpp
  src/mode/katemodemenulist.h

To: nibags, cullmann, #ktexteditor
Cc: ngraham, anthonyfieroni, kwrite-devel, kde-frameworks-devel, rrosch, 
LeGast00n, GB_2, domson, michaelh, bruns, demsking, cullmann, sars, dhaumann

D26167: Update holidays and add flagdays and namedays for Sweden

[Albert Astals Cid]

This revision was automatically updated to reflect the committed changes.
Closed by commit R175:2d9ea97d1cdd: Update holidays and add flagdays and 
namedays for Sweden (authored by riiga, committed by aacid).

REPOSITORY
  R175 KHolidays

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26167?vs=72048&id=74529

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

AFFECTED FILES
  holidays/holidays.qrc
  holidays/plan2/holiday_se_sv
  holidays/plan2/holiday_se_sv_nameday

To: riiga, #kde_pim, winterz
Cc: ltoscano, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

T11627: Improve KIO asynchronicity

[Méven Car]

meven added a comment.


  In T11627#218943 , @meven wrote:
  
  > In T11627#206210 , @broulik 
wrote:
  >
  > > I'm pretty sure there's a tonne of blocking DBus calls being done all 
over the place during file copy (checking for slaves on hold, updating 
kuiserver job tracking info, etc) which can cause apps to hang or slow down if 
whatever they're trying to call into is busy. Best example: SIGSTOP 
plasmashell, try copying a file in Dolphin, won't work until plasmashell 
SIGCONTs.
  >
  >
  > That's interesting, we need to fix that.
  >
  > About https://bugs.kde.org/show_bug.cgi?id=281270 `Inconsistent 
notifications during/after file operations (copying, moving, deleting, 
compressing, extracting) depending on amount of data/items involved` :
  >
  > A possible way to fix this would probably use sendfile with O_NONBLOCK + 
select.
  >  After a non-blocking sendfile we call select/pselect on the dest file 
descriptor. Then when it returns something other than EWOULDBLOCK, we can 
deduce the processed bytes from the size sent to copy to dest (i.e sendfile 
count parameter) or use pselect and its sigset_t parameter. We then can call 
sendfile again to write next bytes.
  >  On the plus side it would make the code async which would make progress 
reporting more reactive.
  
  
  This approach does not work unfortunately, select does not take into account 
filesystem sync.
  I tried also with `sync_file_range` but it does not block when file is 
syncing.
  
  That being said the select method allows to free some main thread cpu time 
while the kernel is running sendfile.
  Allowing for instance to notify ioslaves users of progress.
  
  Regarding this bug I have the idea of adding a state "Closing file" or 
"Writing data to disk" to the CopyJob so users can surface to users that 
something is going on.
  
  And I want to edit D25079  where the 
chunk size for sendfile is probably too big.

TASK DETAIL
  https://phabricator.kde.org/T11627

To: meven
Cc: broulik, ognarb, #dolphin, #frameworks, meven, dfaure, pberestov, iasensio, 
fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D26951: Remove old code that is not used anymore

[Kevin Ottens]

ervin added a comment.


  LGTM, better wait for @dfaure opinion though

REPOSITORY
  R265 KConfigWidgets

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

To: davidre, dfaure, ervin, #frameworks
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26133: Enable Auto Save

[Kevin Ottens]

ervin added a comment.


  Maybe look at KConfigCompilerSignallingItem? But I don't see another way of 
doing it than a wrapper approach like KConfigCompilerSignallingItem does... but 
then that's why ManagedConfigModule assumes GenerateProperties=true in the 
kcfgc, otherwise we'd have hacked the compiler to do the same thing anyway just 
inconditionnally...
  
  That's why personally I think it's fine to assume people need to opt-in for 
GenerateProperties if they want the feature, it's closely related after all.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin
Cc: ervin, GB_2, apol, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D26867: New class: KConfigTypeInformation

[Kevin Ottens]

ervin added a comment.


  Not sure if we want it in this patch or later on, but wouldn't it be better 
to have Param::type and CfgEntry::type be of KConfigTypeInformation type? We 
would then convert early and avoid all those ctor calls to 
KConfigTypeInformation spread in the code. I suspect it'd improve readability.

REPOSITORY
  R237 KConfig

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

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

D26868: Move newItem to private method in KConfigSourceGenerator

[Friedrich W. H. Kossebau]

kossebau added a comment.


  And more while at it: for latin1 strings used in concatenation it is 
recommended to use QLatin1String and for single chars QLatin1Char. 
QStringLiteral is more expensive here, and QStringBuilder & QString has proper 
overloads for the concatenations. So you want to keep the QLatin1Strings and 
rather turn those for single chars to QLatin1Char.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin, dfaure
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26868: Move newItem to private method in KConfigSourceGenerator

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> KConfigSourceGenerator.cpp:659
> +
> +QString KConfigSourceGenerator::newItem(const CfgEntry* entry, const QString 
> &key, const QString& defaultValue, const QString ¶m)
> +{

Since we're here, what about fixing the coding style? (same thing for the 
declaration)

REPOSITORY
  R237 KConfig

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

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

D26876: Remove the Enum hack: finish lists with a comma, it's valid c++

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> KConfigHeaderGenerator.cpp:252
>  
> -// HACK: Use C-Style for add a comma in all but the last element,
> -// just to make the source generated code equal to the old one.
> -// When we are sure, revert this to a range-based-for and just add
> -// a last comma, as it's valid c++.
> -for (int i = 0, end = parseResult.signalList.size(); i < end; i++) {
> -auto signal = parseResult.signalList.at(i);
> -stream() << whitespace() << "  " << signalEnumName(signal.name) << " 
> = 0x" << hex << val;
> -if (i != end-1) {
> -stream() << ",\n";
> -}
> -
> +for (const auto signal : parseResult.signalList) {
> +stream() << whitespace() << "  " << signalEnumName(signal.name) << " 
> = 0x" << hex << val << ",\n";

I lost track of constness here so I might be wrong, shouldn't this use qAsConst 
in this context?

REPOSITORY
  R237 KConfig

BRANCH
  remove_enum_hack

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

To: tcanabrava, ervin, dfaure, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26877: Simplify calls to whitespace() and use it in more places.

[Kevin Ottens]

ervin added a comment.


  I'm not sure I like whitespace() returning the stream and then being used. In 
effect it leads to hiding the stream object making the code more terse indeed 
but less obvious I think.
  Maybe it's a question of personal taste, @dfaure any opinion?

REPOSITORY
  R237 KConfig

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

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

D26968: Removed hardcoded colors in background SVGs

[Niccolò Venerandi]

niccolove added a comment.


  In D26968#602035 , @ndavis wrote:
  
  > In D26968#602034 , @niccolove 
wrote:
  >
  > > Okay! I'm installing Inkscape 1, for future patches. May I ask you to 
land this, as I don't have access rights?
  >
  >
  > Sure. Do you not have a KDE developer account?
  
  
  I just checked and I do, I probably made some mistake with the shh publickey 
setup, I will try to fix that

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D26968: Removed hardcoded colors in background SVGs

[Noah Davis]

ndavis retitled this revision from "Removed hardcoded values" to "Removed 
hardcoded colors in background SVGs".

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  fix_panel_color

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

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

D26968: Removed hardcoded colors in background SVGs

[Noah Davis]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:ffbd186975cf: Removed hardcoded colors in background SVGs 
(authored by niccolove, committed by ndavis).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26968?vs=74511&id=74519

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

AFFECTED FILES
  src/desktoptheme/breeze/translucent/widgets/panel-background.svg
  src/desktoptheme/breeze/translucent/widgets/tooltip.svg

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

D26968: Removed hardcoded values

[Noah Davis]

ndavis added a comment.


  In D26968#602034 , @niccolove 
wrote:
  
  > Okay! I'm installing Inkscape 1, for future patches. May I ask you to land 
this, as I don't have access rights?
  
  
  Sure. Do you not have a KDE developer account?

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  fix_panel_color

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

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

D26968: Removed hardcoded values

[Niccolò Venerandi]

niccolove added a comment.


  Okay! I'm installing Inkscape 1, for future patches. May I ask you to land 
this, as I don't have access rights?

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  fix_panel_color

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

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

D26968: Removed hardcoded values

[Noah Davis]

ndavis added a comment.


  Anyway, it works as it is, so don't worry about it.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  fix_panel_color

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

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

D26968: Removed hardcoded values

[Noah Davis]

ndavis added a comment.


  In D26968#602031 , @niccolove 
wrote:
  
  > In D26968#601998 , @ndavis wrote:
  >
  > > You should remove the color field instead of using `color:currentColor` 
and `fill:currentColor`
  >
  >
  > I tried to do that, but for some reason the "color:#effeff" automatically 
appears again (in inkscape) :-/
  
  
  Yeah, you have to remove it with `sed` or a text editor every time unless you 
use Inkscape 1.0

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  fix_panel_color

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

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

D26968: Removed hardcoded values

[Niccolò Venerandi]

niccolove added a comment.


  In D26968#601998 , @ndavis wrote:
  
  > You should remove the color field instead of using `color:currentColor` and 
`fill:currentColor`
  
  
  I tried to do that, but for some reason the "color:#effeff" automatically 
appears again (in inkscape) :-/

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  fix_panel_color

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

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

D21466: Recommend rebooting after installing Samba

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> ngraham wrote in sambausershareplugin.h:63
> Yes, but I'd prefer to do that in another patch and port all the inline 
> messages at once.

Ah! There's more. Fine with me then.

REPOSITORY
  R432 File Sharing (Samba) integration

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

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

D21466: Recommend rebooting after installing Samba

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> sitter wrote in sambausershareplugin.h:63
> shouldn't this rather be a KMessageWidget? As I recall we usually use KMWs 
> for this type of notification.

Yes, but I'd prefer to do that in another patch and port all the inline 
messages at once.

REPOSITORY
  R432 File Sharing (Samba) integration

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

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

D26968: Removed hardcoded values

[Noah Davis]

ndavis added reviewers: VDG, Plasma.
ndavis added a comment.


  This does have rounded corners and the background is fixed.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D26968: Removed hardcoded values

[Noah Davis]

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

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  fix_panel_color

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

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

D26961: Be more helpfully verbose when we can't start an action

[Nathaniel Graham]

ngraham accepted this revision.

REPOSITORY
  R283 KAuth

BRANCH
  master

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

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

D26968: Removed hardcoded values

[Noah Davis]

ndavis added a comment.


  You should remove the color field instead of using `color:currentColor` and 
`fill:currentColor`

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D26957: KateModeMenuList: don't overlap the scroll bar

[Nathaniel Graham]

ngraham added a comment.


  JFYI Kate has moved to GitLab and most patches are now submitted at 
https://invent.kde.org/kde/kate.

REPOSITORY
  R39 KTextEditor

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

To: nibags, cullmann, #ktexteditor
Cc: ngraham, anthonyfieroni, kwrite-devel, kde-frameworks-devel, rrosch, 
LeGast00n, GB_2, domson, michaelh, bruns, demsking, cullmann, sars, dhaumann

D26955: Fix background SVG colors and adjust background SVG corners

[Noah Davis]

ndavis added a comment.


  In D26955#601984 , @niccolove 
wrote:
  
  > Might the problems of not-rounded corners here be caused from the "adjust 
background SVG corners" part by any chance?
  
  
  I tried using the same corners as the shadow patch, but it didn't change 
anything.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

KDE CI: Frameworks » extra-cmake-modules » kf5-qt5 SUSEQt5.12 - Build # 90 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/extra-cmake-modules/job/kf5-qt5%20SUSEQt5.12/90/
 Project:
kf5-qt5 SUSEQt5.12
 Date of build:
Tue, 28 Jan 2020 16:26:57 +
 Build duration:
3 min 59 sec and counting
   JUnit Tests
  Name: projectroot Failed: 2 test(s), Passed: 18 test(s), Skipped: 0 test(s), Total: 20 test(s)Failed: projectroot.tests.ECMPoQmToolsTestFailed: projectroot.tests.GenerateSipBindingsName: projectroot.tests Failed: 0 test(s), Passed: 61 test(s), Skipped: 0 test(s), Total: 61 test(s)Name: projectroot.tests.ECMAddTests Failed: 0 test(s), Passed: 2 test(s), Skipped: 0 test(s), Total: 2 test(s)
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  Cobertura Coverage Report

KDE CI: Frameworks » extra-cmake-modules » kf5-qt5 SUSEQt5.13 - Build # 56 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/extra-cmake-modules/job/kf5-qt5%20SUSEQt5.13/56/
 Project:
kf5-qt5 SUSEQt5.13
 Date of build:
Tue, 28 Jan 2020 16:26:58 +
 Build duration:
3 min 16 sec and counting
   JUnit Tests
  Name: projectroot Failed: 2 test(s), Passed: 18 test(s), Skipped: 0 test(s), Total: 20 test(s)Failed: projectroot.tests.ECMPoQmToolsTestFailed: projectroot.tests.GenerateSipBindingsName: projectroot.tests Failed: 0 test(s), Passed: 61 test(s), Skipped: 0 test(s), Total: 61 test(s)Name: projectroot.tests.ECMAddTests Failed: 0 test(s), Passed: 2 test(s), Skipped: 0 test(s), Total: 2 test(s)
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  Cobertura Coverage Report

D26969: fix type namespace requirement

[Harald Sitter]

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

REVISION SUMMARY
  it was hard to find in all the text anyway. and it's an entirely
  pointless requirement. we can easily determine the type name dynamically
  and call with or without namespace as necessary. this way users of kauth
  cannot make the mistake of not using the namespace by accident.
  
  I've thought about using a generic return argument (passing the typeName
  into QGenericReturnArgument directly) but I think that opens the helper
  code to memory corruption should they make the mistake of returning an
  entirely unexpected type. so, what I ended up with is a bit more code
  unfortunately.

TEST PLAN
  helper code works with both `using namespace` and without

REPOSITORY
  R283 KAuth

BRANCH
  master

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

AFFECTED FILES
  src/backends/dbus/DBusHelperProxy.cpp
  src/kauthactionreply.h

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

D26573: Add missing Import Env Variable

[Tomaz Canabrava]

This revision was automatically updated to reflect the committed changes.
Closed by commit R240:ee0531749057: Add missing Import Env Variable (authored 
by tcanabrava).

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26573?vs=74512&id=74513

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

AFFECTED FILES
  kde-modules/prefix.sh.cmake

To: tcanabrava, mart
Cc: apol, davidedmundson, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
GB_2, bencreasy, michaelh, ngraham, bruns

D26955: Fix background SVG colors and adjust background SVG corners

[Niccolò Venerandi]

niccolove added a comment.


  Might the problems of not-rounded corners here be caused from the "adjust 
background SVG corners" part by any chance?

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D26573: Add missing Import Env Variable

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 74512.
tcanabrava added a comment.


  - Fix Variable

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26573?vs=73237&id=74512

BRANCH
  add_missing_env_var

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

AFFECTED FILES
  kde-modules/prefix.sh.cmake

To: tcanabrava, mart
Cc: apol, davidedmundson, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
GB_2, bencreasy, michaelh, ngraham, bruns

D26968: Removed hardcoded values

[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
  Alternative and more basic patch to D26955 
, this aims to fix the background color 
without breaking the mask

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  fix_panel_color

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

AFFECTED FILES
  src/desktoptheme/breeze/translucent/widgets/panel-background.svg
  src/desktoptheme/breeze/translucent/widgets/tooltip.svg

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

D26131: Braces around for, break early.

[Kevin Ottens]

ervin added a comment.


  Does it apply on top of your refactoring? Also the description looks wrong 
now.

INLINE COMMENTS

> kconfig_compiler.cpp:179
>  {
> -bool isAscii = true;
> -for (int i = s.length(); i--;)
> -if (s[i].unicode() > 127) {
> -isAscii = false;
> -}
> -
> +bool isAscii = ! std::any_of(std::begin(s), std::end(s), [](QChar a) { 
> return a.unicode() > 127; });
>  if (isAscii) {

Nitpick: there shouldn't be a space after !

Also: use std::cbegin and std::cend (I think they're allowed nowadays)
And: "const QChar &" instead of QChar?

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin, dfaure
Cc: ervin, patrickelectric, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26573: Add missing Import Env Variable

[Aleix Pol Gonzalez]

apol added a comment.


  I don't really understand why this is needed, I've never needed that at least.
  
  Having @davidedmundson's patch would be interesting too, although we might 
want to support earlier versions that won't get the patch.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  add_missing_env_var

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

To: tcanabrava, mart
Cc: apol, davidedmundson, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
GB_2, bencreasy, michaelh, ngraham, bruns

D26573: Add missing Import Env Variable

[David Edmundson]

davidedmundson added a comment.


  I'm now affected by this :(
  
  I think this shouldn't be needed, but I have no objections to merging this 
either.
  
  https://codereview.qt-project.org/c/qt/qtquickcontrols2/+/288238

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  add_missing_env_var

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

To: tcanabrava, mart
Cc: davidedmundson, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

Re: SPDX KAboutLicense

[Harald Sitter]

On Tue, Jan 28, 2020 at 3:06 PM Aleix Pol  wrote:
>
> On Tue, Jan 28, 2020 at 12:29 PM Harald Sitter  wrote:
> >
> > Hey!
> >
> > For general interest, and so this thought doesn't only exist in my
> > head: As a follow-up to us embracing SPDX for code files it would make
> > a whole lot of sense sense to move KAboutLicense away from our custom
> > text format (see KAboutLicense::byKeyword) to SPDX as well. The text
> > format is particularly used in plugin and plasma applets
> > metadata/desktop files and while we managed to patch on a semblance of
> > spdx support by simply hardcoding mapping rules it's super limited
> > because we are constricted by the actual feature set of KAL, which
> > among other things means license expressions are not a thing at all
> > (e.g. [1]) which in turn then limits the license definition I can use
> > for my plasma applets.
> >
> > I reckon this can be pulled off backwards compatible, seeing as we
> > have retrofitted KAL with spdx license tag support we can "up-cast" a
> > KAL to a SpdxExpression.
>
> +1 for overall SPDX adoption.
>
> I'd say we need to keep the licences texts for --version in applications.

In fact we can easily expand on them even! The spdx.org data is
generated from machine readable definitions files and we could simply
ship a select view for local resolution
https://github.com/spdx/license-list-data/tree/master/text KAboutData
then simply needs to make a choice on how to represent each license.
One that has a local copy can just be dumped to stdout, ones which
cannot be resolved locally simply gets a URL to spdx.org. So as a
third party author using kaboutdata I can use "GPL-2.0 OR X11" and
--version will spit out the GPL2 (because it's one of ours, so we'd
ship a copy I expect) while linking to spdx.org for the X11 (because
we don't use it so we'd not ship a copy). Taking it a step further if
I make a windows installable I can kick out all copies or just ship
the ones applicable to my application.
(All of this is equally applicable to KAboutDialog, which can also
show the full license text)

HS

Re: SPDX KAboutLicense

[Aleix Pol]

On Tue, Jan 28, 2020 at 12:29 PM Harald Sitter  wrote:
>
> Hey!
>
> For general interest, and so this thought doesn't only exist in my
> head: As a follow-up to us embracing SPDX for code files it would make
> a whole lot of sense sense to move KAboutLicense away from our custom
> text format (see KAboutLicense::byKeyword) to SPDX as well. The text
> format is particularly used in plugin and plasma applets
> metadata/desktop files and while we managed to patch on a semblance of
> spdx support by simply hardcoding mapping rules it's super limited
> because we are constricted by the actual feature set of KAL, which
> among other things means license expressions are not a thing at all
> (e.g. [1]) which in turn then limits the license definition I can use
> for my plasma applets.
>
> I reckon this can be pulled off backwards compatible, seeing as we
> have retrofitted KAL with spdx license tag support we can "up-cast" a
> KAL to a SpdxExpression.

+1 for overall SPDX adoption.

I'd say we need to keep the licences texts for --version in applications.

Aleix

D26957: KateModeMenuList: don't overlap the scroll bar

[Anthony Fieroni]

anthonyfieroni added a comment.


  Don't remove default parameter, it'll break other users.

REPOSITORY
  R39 KTextEditor

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

To: nibags, cullmann, #ktexteditor
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, 
GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D21466: Recommend rebooting after installing Samba

[Harald Sitter]

sitter added a comment.


  random comment: since we install through polkit the distro should deal with 
this. polkit can report whether a reboot is necessary, and discover has support 
for that, so in theory if a distro/polkit backend reports it as necessary 
(because they added a new group), it should report that to polkit so discover 
can notify the user. so, maaaybe this shouldn't actually be decided by us 
at all. now whether or not we can trust distros to actually take care of this 
properly is another matter entirely
  
  as for the patch: KSambaShare (part of KIO) ought to get a new function `bool 
userCanChange()` or some such which simply runs `return 
QFileInfo(userSharePath).isWritable()`. i.e. to determine whether or not a 
reboot is (like) required you simply need to check whether the user can 
currently write to the directory where the user shares are stored.
  I don't think that is really blocking the diff though. it's a refinement to 
be sure, but given the reboot notification can be entirely ignored it's hardly 
a critical blocker for this diff.
  
  LGTM, but I'd prefer if @apol weighs in.

INLINE COMMENTS

> sambausershareplugin.h:63
> +#ifdef SAMBA_INSTALL
> +QWidget *m_justInstalledSambaWidgets;
> +QPushButton *m_restartButton;

shouldn't this rather be a KMessageWidget? As I recall we usually use KMWs for 
this type of notification.

REPOSITORY
  R432 File Sharing (Samba) integration

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

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

D26961: Be more helpfully verbose when we can't start an action

[Tomaz Canabrava]

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

REPOSITORY
  R283 KAuth

BRANCH
  master

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

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

D26960: RFC: [KWindowShadows] Check for X connection

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R278:21d67e31f718: [KWindowShadows] Check for X connection 
(authored by broulik).

REPOSITORY
  R278 KWindowSystem

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26960?vs=74486&id=74489

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

AFFECTED FILES
  src/platforms/xcb/kwindowshadow.cpp

To: broulik, #plasma, zzag, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26961: Be more helpfully verbose when we can't start an action

[David Edmundson]

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

REVISION SUMMARY
  If we show what action it tried to start someone can run some more
  useful diagnostics.

TEST PLAN
  Compiles

REPOSITORY
  R283 KAuth

BRANCH
  master

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

AFFECTED FILES
  src/kauthexecutejob.cpp

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

D26960: RFC: [KWindowShadows] Check for X connection

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added a comment.


  Ok, panel shadows is a singleton. That explains things.

REPOSITORY
  R278 KWindowSystem

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

To: broulik, #plasma, zzag, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26960: RFC: [KWindowShadows] Check for X connection

[David Edmundson]

davidedmundson added a comment.


  Heh, I'm going to quote a conversation with you:
  
  
  
  David Edmundson, [17.01.20 10:07]
  would you say it's always wrong to have a QWindow outlive a QApplication?
  
  Kai Uwe, [17.01.20 10:08]
  I would say yes, why?
  
  David Edmundson, [17.01.20 10:08]
  wondering if I need to comment on some code that would crash if that happened
  
  Kai Uwe, [17.01.20 10:13]
  when qapp is gone the qpt and stuff will be torn down and then it will 
naturally crash if a window outlives it, no?
  
  Then you went and proved that windows are torn down.
  
  
  
  If we have a tile alive it means either a KWindowShadow object is alive or 
the QStyle is still alive, and both should be destroyed before the QPA.
  Do you have the full bt of the crash?

REPOSITORY
  R278 KWindowSystem

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

To: broulik, #plasma, zzag
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26960: RFC: [KWindowShadows] Check for X connection

[Vlad Zahorodnii]

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

REPOSITORY
  R278 KWindowSystem

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

To: broulik, #plasma, zzag
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26960: RFC: [KWindowShadows] Check for X connection

[Kai Uwe Broulik]

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

REVISION SUMMARY
  On teardown of plasmashell the shared pointer with shadows is unref'd quite 
late at which point `QX11Info::connection()` already return null, leading to a 
crash in those xcb calls.
  The old code in Plasma PanelShadows checked for `QX11Info::display()` being 
null, which is restored here.

TEST PLAN
  Not sure if this is a good idea? is X smart enough to clean up after us?
  
  - Still crashes in Breeze style but no longer without it

REPOSITORY
  R278 KWindowSystem

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

AFFECTED FILES
  src/platforms/xcb/kwindowshadow.cpp

To: broulik, #plasma, zzag
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

SPDX KAboutLicense

[Harald Sitter]

Hey!

For general interest, and so this thought doesn't only exist in my
head: As a follow-up to us embracing SPDX for code files it would make
a whole lot of sense sense to move KAboutLicense away from our custom
text format (see KAboutLicense::byKeyword) to SPDX as well. The text
format is particularly used in plugin and plasma applets
metadata/desktop files and while we managed to patch on a semblance of
spdx support by simply hardcoding mapping rules it's super limited
because we are constricted by the actual feature set of KAL, which
among other things means license expressions are not a thing at all
(e.g. [1]) which in turn then limits the license definition I can use
for my plasma applets.

I reckon this can be pulled off backwards compatible, seeing as we
have retrofitted KAL with spdx license tag support we can "up-cast" a
KAL to a SpdxExpression.

Food for thought.

[1] https://phabricator.kde.org/D6672

HS

D26858: Provide an implementation for the tablet interface

[Vlad Zahorodnii]

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


  Code-wise, this change looks good. I don't have a tablet device to test this 
patch so you may want to wait for a +1 from @davidedmundson.

INLINE COMMENTS

> tablet_interface.cpp:369
> +void zwp_tablet_manager_v2_get_tablet_seat(Resource *resource, uint32_t 
> tablet_seat,
> +   struct ::wl_resource 
> *seat_resource) override {
> +SeatInterface* seat = SeatInterface::get(seat_resource);

The opening brace must be on its own line.

> tablet_interface.cpp:370
> +   struct ::wl_resource 
> *seat_resource) override {
> +SeatInterface* seat = SeatInterface::get(seat_resource);
> +TabletSeatInterface *tsi = get(seat);

SeatInterface *seat = SeatInterface::get(seat_resource);

REPOSITORY
  R127 KWayland

BRANCH
  apol/tablet-unstable-v2-1

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

To: apol, #kwin, #frameworks, zzag
Cc: davidedmundson, zzag, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26758: Fix the size and pixel alignment of checkboxes and radiobuttons

[George Vogiatzis]

gvgeo added a comment.


  Plasma theme explorer is fine. The difference is that it uses 
plasmaComponents, while D26271  uses 
plasmaComponents**3**.
  
  About plasmaComponents:
  There is some bug with the checkbox and the Label. Fails to take the correct 
size (uses paintedHeight).
  
height: Math.round(Math.max(paintedHeight, 
theme.mSize(theme.defaultFont).height*1.6))
  
  Radiobutton draws correctly the label height. But need indicator vertical 
placement or different label height.
  
  
  
  Non relevant note: 
  There is a workaround in multiple places for 
  https://bugreports.qt.io/browse/QTBUG-67007
  Which was fixed in Qt 5.14 from
  https://bugreports.qt.io/browse/QTBUG-70481
  Should be removed or need to stay awhile?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ndavis, #plasma, #vdg, ngraham
Cc: gvgeo, ngraham, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D26957: KateModeMenuList: don't overlap the scroll bar

[Nibaldo González]

nibags edited the summary of this revision.
nibags added reviewers: cullmann, KTextEditor.

REPOSITORY
  R39 KTextEditor

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

To: nibags, cullmann, #ktexteditor
Cc: kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, GB_2, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D26957: KateModeMenuList: don't overlap the scroll bar

[Nibaldo González]

nibags created this revision.
Herald added projects: Kate, Frameworks.
Herald added subscribers: kde-frameworks-devel, kwrite-devel.
nibags requested review of this revision.

REPOSITORY
  R39 KTextEditor

BRANCH
  mode-menu-scrollbar-overlap

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

AFFECTED FILES
  src/mode/katemodemenulist.cpp
  src/mode/katemodemenulist.h

To: nibags
Cc: kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, GB_2, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann