D6820: Add QValidator to KTimeCombobox

[Christoph Feck]

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

INLINE COMMENTS

> ktimecombobox.cpp:33
> +public:
> +KTimeValidator(QString timeFormat, QObject *parent = Q_NULLPTR);
> +KTimeValidator(QString timeFormat, QTime min, QTime max, QObject *parent 
> = Q_NULLPTR);

Please use "const &" for all QTime and QString arguments.

> ktimecombobox.cpp:34
> +KTimeValidator(QString timeFormat, QObject *parent = Q_NULLPTR);
> +KTimeValidator(QString timeFormat, QTime min, QTime max, QObject *parent 
> = Q_NULLPTR);
> +~KTimeValidator();

Since "min" and "max" might be macros for some compilers, please rename to 
"minTime" and "maxTime".

> ktimecombobox.cpp:37
> +virtual State validate(QString , int ) const Q_DECL_OVERRIDE;
> +void setMin(QTime min);
> +void setMax(QTime max);

setMinTime(const QTime );

It looks more verbose, but also more correct :)

> ktimecombobox.cpp:42
> +QString m_timeFormat;
> +QTime m_min, m_max;
> +};

Also here, m_minTime and m_maxTime.

> ktimecombobox.cpp:76
> +Q_UNUSED(pos);
> +QTime t = QTime::fromString(input, m_timeFormat);
> +if (t.isValid()) {

const QTime time = ...

> ktimecombobox.cpp:231
> +m_validator = new KTimeValidator(locale.timeFormat(m_displayFormat),
> + m_minTime, m_maxTime);
> +q->lineEdit()->setValidator(m_validator);

So the KTimeComboBox class already has m_min/maxTime members. Can the code be 
refactored that they do not need to be stored twice?

REPOSITORY
  R236 KWidgetsAddons

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

To: bednar, cfeck
Cc: cfeck, #frameworks

D6820: Add QValidator to KTimeCombobox

[Christoph Feck]

cfeck added a comment.


  Are you sure the patch is complete? I remember there was code which was 
supposed to be replaced by the new validator.

INLINE COMMENTS

> ktimecombobox.cpp:608
>  #include "moc_ktimecombobox.cpp"
> +#include "ktimecombobox.moc"

This does not look right.

REPOSITORY
  R236 KWidgetsAddons

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

To: bednar
Cc: cfeck, #frameworks

D7062: Create a designer plugin for kpasswordlineedit

[Laurent Montel]

mlaurent closed this revision.

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

To: mlaurent, dfaure, kossebau, cfeck
Cc: cfeck, elvisangelaccio, #frameworks

D7103: Allow to build KSyntaxHighlighter without Qt5Gui

[Kevin Funk]

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

INLINE COMMENTS

> CMakeLists.txt:3
> +if(TARGET Qt5::Gui)
> +add_subdirectory(lib)
> +add_subdirectory(cli)

Too bad it needs QtGui. Just b/c it uses `QColor` as far as I can see...

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  master

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

To: vkrause, #frameworks, kfunk
Cc: kfunk

D7102: Add cross-compilation support for the highlighting indexer

[Kevin Funk]

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


  TIL you can use `IMPORTED` on `add_executable`...
  
  LGTM

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  hl-cross-compile

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

To: vkrause, #frameworks, kfunk
Cc: kfunk

D7117: Fix compilation on Yocto

[Volker Krause]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:0a8d1f9cd474: Fix compilation on Yocto (authored by 
vkrause).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7117?vs=17661=17677

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

AFFECTED FILES
  src/declarativeimports/core/colorscope.cpp

To: vkrause, #frameworks, broulik

D7117: Fix compilation on Yocto

[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/D7117

To: vkrause, #frameworks, broulik

D7117: Fix compilation on Yocto

[Volker Krause]

vkrause created this revision.
Restricted Application added a project: Frameworks.

REVISION SUMMARY
  By default, Qt is built without accessibility there, which is what
  would indirectly include the needed QColor with a "normal" Qt build
  here.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/core/colorscope.cpp

To: vkrause, #frameworks

D7102: Add cross-compilation support for the highlighting indexer

[Volker Krause]

vkrause created this revision.
Restricted Application added a project: Frameworks.

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  hl-cross-compile

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

AFFECTED FILES
  src/indexer/CMakeLists.txt

To: vkrause, #frameworks

D7010: KSqueezedTextLabel: call updateGeometry() when text changes

[Sven Brauch]

brauch updated this revision to Diff 17638.
brauch added a comment.


  Right. Better call it here.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7010?vs=17411=17638

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

AFFECTED FILES
  src/ksqueezedtextlabel.cpp

To: brauch, cfeck
Cc: aacid, #frameworks

D7010: KSqueezedTextLabel: call updateGeometry() when text changes

[Christoph Feck]

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


  The title says "when text changes", but you call it even if the text does not 
change. Note that squeezeTextToLabel() is called from many places, including 
resizeEvent(), so avoid calling updateGeometry() if not needed.

REPOSITORY
  R236 KWidgetsAddons

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

To: brauch, cfeck
Cc: aacid, #frameworks

D7011: Extract KPasswordLineEdit class

[Christoph Feck]

cfeck removed a reviewer: elvisangelaccio.
This revision is now accepted and ready to land.

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

To: mlaurent, cfeck, dfaure
Cc: kfunk, kossebau, elvisangelaccio, #frameworks

D7011: Extract KPasswordLineEdit class

[Christoph Feck]

cfeck accepted this revision.
cfeck added a comment.


  Together with the Designer stuff, please.

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

To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kfunk, kossebau, elvisangelaccio, #frameworks

KDE CI: Frameworks kirigami kf5-qt5 FreeBSDQt5.7 - Build # 31 - Still Unstable!

[no-reply]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kirigami%20kf5-qt5%20FreeBSDQt5.7/31/
 Project:
Frameworks kirigami kf5-qt5 FreeBSDQt5.7
 Date of build:
Thu, 03 Aug 2017 15:30:46 +
 Build duration:
1 min 8 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 0 test(s), Skipped: 0 test(s), Total: 1 test(s)Failed: TestSuite.qmltests

build.log
Description: Binary data

KDE CI: Frameworks kirigami kf5-qt5 XenialQt5.7 - Build # 33 - Still Unstable!

[no-reply]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kirigami%20kf5-qt5%20XenialQt5.7/33/
 Project:
Frameworks kirigami kf5-qt5 XenialQt5.7
 Date of build:
Thu, 03 Aug 2017 15:30:46 +
 Build duration:
1 min 0 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 2 test(s)Failed: TestSuite.qmltests
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  Cobertura Coverage Report

build.log
Description: Binary data

KDE CI: Frameworks kirigami kf5-qt5 XenialQt5.7 - Build # 32 - Still Unstable!

[no-reply]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kirigami%20kf5-qt5%20XenialQt5.7/32/
 Project:
Frameworks kirigami kf5-qt5 XenialQt5.7
 Date of build:
Thu, 03 Aug 2017 14:49:51 +
 Build duration:
58 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 2 test(s)Failed: TestSuite.qmltests
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  Cobertura Coverage Report

build.log
Description: Binary data

KDE CI: Frameworks kirigami kf5-qt5 XenialQt5.7 - Build # 31 - Still Unstable!

[no-reply]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kirigami%20kf5-qt5%20XenialQt5.7/31/
 Project:
Frameworks kirigami kf5-qt5 XenialQt5.7
 Date of build:
Thu, 03 Aug 2017 14:42:46 +
 Build duration:
52 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 2 test(s)Failed: TestSuite.qmltests
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  Cobertura Coverage Report

build.log
Description: Binary data

KDE CI: Frameworks kirigami kf5-qt5 FreeBSDQt5.7 - Build # 29 - Still Unstable!

[no-reply]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kirigami%20kf5-qt5%20FreeBSDQt5.7/29/
 Project:
Frameworks kirigami kf5-qt5 FreeBSDQt5.7
 Date of build:
Thu, 03 Aug 2017 14:42:46 +
 Build duration:
26 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 0 test(s), Skipped: 0 test(s), Total: 1 test(s)Failed: TestSuite.qmltests

build.log
Description: Binary data

KDE CI: Frameworks kirigami kf5-qt5 XenialQt5.7 - Build # 30 - Still Unstable!

[no-reply]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kirigami%20kf5-qt5%20XenialQt5.7/30/
 Project:
Frameworks kirigami kf5-qt5 XenialQt5.7
 Date of build:
Thu, 03 Aug 2017 14:28:44 +
 Build duration:
4 min 11 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 2 test(s)Failed: TestSuite.qmltests
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  Cobertura Coverage Report

build.log
Description: Binary data

D7058: Initialize the "Pause" button state in the widget tracker

[Aleix Pol Gonzalez]

apol added a comment.


  I just tried now. It works. 

REPOSITORY
  R288 KJobWidgets

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

To: apol, #frameworks
Cc: aacid

KDE CI: Frameworks kirigami kf5-qt5 FreeBSDQt5.7 - Build # 28 - Still Unstable!

[no-reply]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kirigami%20kf5-qt5%20FreeBSDQt5.7/28/
 Project:
Frameworks kirigami kf5-qt5 FreeBSDQt5.7
 Date of build:
Thu, 03 Aug 2017 14:28:44 +
 Build duration:
26 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 0 test(s), Skipped: 0 test(s), Total: 1 test(s)Failed: TestSuite.qmltests

build.log
Description: Binary data

KDE CI: Frameworks kirigami kf5-qt5 XenialQt5.7 - Build # 29 - Still Unstable!

[no-reply]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kirigami%20kf5-qt5%20XenialQt5.7/29/
 Project:
Frameworks kirigami kf5-qt5 XenialQt5.7
 Date of build:
Thu, 03 Aug 2017 14:15:24 +
 Build duration:
4 min 23 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 2 test(s)Failed: TestSuite.qmltests
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  Cobertura Coverage Report

build.log
Description: Binary data

KDE CI: Frameworks kirigami kf5-qt5 FreeBSDQt5.7 - Build # 27 - Still Unstable!

[no-reply]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kirigami%20kf5-qt5%20FreeBSDQt5.7/27/
 Project:
Frameworks kirigami kf5-qt5 FreeBSDQt5.7
 Date of build:
Thu, 03 Aug 2017 14:15:24 +
 Build duration:
48 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 0 test(s), Skipped: 0 test(s), Total: 1 test(s)Failed: TestSuite.qmltests

build.log
Description: Binary data

D7094: Include a module for finding qml imports as runtime dependencies

[Aleix Pol Gonzalez]

apol created this revision.
Restricted Application added projects: Frameworks, Build System.

REVISION SUMMARY
  Allows to check if a module is available on the system and sets it as a
  runtime dependency.
  This is useful for projects so that they can specify their qml dependencies
  easily and packagers and developers get to see what's missing by looking
  at the cmake output.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

AFFECTED FILES
  modules/ECMFindQMLModule.cmake.in
  modules/ECMQMLModules.cmake

To: apol, #build_system, #frameworks, sitter

D7011: Extract KPasswordLineEdit class

[Elvis Angelaccio]

elvisangelaccio added a comment.


  +1, looks good to me now.

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

To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kfunk, kossebau, elvisangelaccio, #frameworks

D7011: Extract KPasswordLineEdit class

[Christoph Feck]

cfeck added a comment.


  Looks good to go from my side. Let's wait a bit if someone has more 
objections.
  
  Thanks Laurent, it was a needed change considering the code duplication that 
was present before.

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

To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kfunk, kossebau, elvisangelaccio, #frameworks

D7011: Extract KPasswordLineEdit class

[Laurent Montel]

mlaurent updated this revision to Diff 17621.
mlaurent added a comment.


  +1

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7011?vs=17620=17621

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/knewpasswordwidgettest.cpp
  autotests/kpassworddialogautotest.cpp
  autotests/kpasswordlineedittest.cpp
  autotests/kpasswordlineedittest.h
  src/CMakeLists.txt
  src/knewpasswordwidget.cpp
  src/knewpasswordwidget.h
  src/knewpasswordwidget.ui
  src/kpassworddialog.cpp
  src/kpassworddialog.h
  src/kpassworddialog.ui
  src/kpasswordlineedit.cpp
  src/kpasswordlineedit.h
  tests/CMakeLists.txt
  tests/kpasswordlineedit_test.cpp

To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kfunk, kossebau, elvisangelaccio, #frameworks

D7011: Extract KPasswordLineEdit class

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kfunk wrote in kpasswordlineedit.cpp:30
> +1. Don't use nested private classes. Nested classes/structs inherit the 
> symbols visibility.
> 
> Also see: https://phabricator.kde.org/D6927
> 
> PS: In times of Qt5 & lambda-connects, you don't need Q_PRIVATE_SLOT anymore, 
> anyway: 
> Just remove the `Q_PRIVATE_SLOT` marker and ...:
> `connect(..., this, [this](...) { d->myPrivateSlot(...); });`

Oh, that would make a nice "junior job" to get those cleaned out everywhere. Is 
removing a private slot considered API/ABI compatible?

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

To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kfunk, kossebau, elvisangelaccio, #frameworks

D7011: Extract KPasswordLineEdit class

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kpasswordlineedit.cpp:3
> +   Copyright (c) 2017 Montel Laurent 
> +
> +   This library is free software; you can redistribute it and/or modify

Elvis, you wrote the "visibilityAction" stuff, right? If yes, needs to be 
mentioned here.

> kpasswordlineedit.cpp:26
> +#include 
> +#include 
> +#include 

QLineEdit is included in the .h file, can be removed here.

> kpasswordlineedit.cpp:36
> +{
> +
> +}

Remove empty line.

> kpasswordlineedit.cpp:60
> +
> +
> +q->connect(toggleEchoModeAction, ::triggered, q, [this]() 
> {_k_echoModeToggled();});

Remove doubled empty line.

> kpasswordlineedit.cpp:116
> +d->passwordLineEdit->setText(password);
> +Q_EMIT passwordChanged(password);
> +}

Does this need a protection for not emitting if the value did not actually 
change? I think it helps to prevent recursion.

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

To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kfunk, kossebau, elvisangelaccio, #frameworks

KDE CI: Frameworks kirigami kf5-qt5 XenialQt5.7 - Build # 28 - Still Unstable!

[no-reply]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kirigami%20kf5-qt5%20XenialQt5.7/28/
 Project:
Frameworks kirigami kf5-qt5 XenialQt5.7
 Date of build:
Thu, 03 Aug 2017 11:20:24 +
 Build duration:
1 min 9 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 2 test(s)Failed: TestSuite.qmltests
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  Cobertura Coverage Report

build.log
Description: Binary data

KDE CI: Frameworks kirigami kf5-qt5 FreeBSDQt5.7 - Build # 26 - Still Unstable!

[no-reply]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kirigami%20kf5-qt5%20FreeBSDQt5.7/26/
 Project:
Frameworks kirigami kf5-qt5 FreeBSDQt5.7
 Date of build:
Thu, 03 Aug 2017 11:20:24 +
 Build duration:
48 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 0 test(s), Skipped: 0 test(s), Total: 1 test(s)Failed: TestSuite.qmltests

build.log
Description: Binary data

D7011: Extract KPasswordLineEdit class

[Laurent Montel]

mlaurent added inline comments.

INLINE COMMENTS

> cfeck wrote in kpasswordlineedit.cpp:30
> That's fishy. I just checked a single class (KColumnResizer), and it has just 
> "class KColumnResizerPrivate" (no namespace, no QObject), and still has a 
> Q_PRIVATE_SLOT in its KColumnResizer class.

wierd it didn't want to build, but after a make clean it works...
next patch will come soon (12)

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

To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kossebau, elvisangelaccio, #frameworks

D7011: Extract KPasswordLineEdit class

[Kevin Funk]

kfunk added inline comments.

INLINE COMMENTS

> cfeck wrote in kpasswordlineedit.cpp:30
> That's fishy. I just checked a single class (KColumnResizer), and it has just 
> "class KColumnResizerPrivate" (no namespace, no QObject), and still has a 
> Q_PRIVATE_SLOT in its KColumnResizer class.

+1. Don't use nested private classes. Nested classes/structs inherit the 
symbols visibility.

Also see: https://phabricator.kde.org/D6927

PS: In times of Qt5 & lambda-connects, you don't need Q_PRIVATE_SLOT anymore, 
anyway: 
Just remove the `Q_PRIVATE_SLOT` marker and ...:
`connect(..., this, [this](...) { d->myPrivateSlot(...); });`

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

To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kfunk, kossebau, elvisangelaccio, #frameworks

KDE CI: Frameworks kpackage kf5-qt5 XenialQt5.7 - Build # 17 - Unstable!

[no-reply]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kpackage%20kf5-qt5%20XenialQt5.7/17/
 Project:
Frameworks kpackage kf5-qt5 XenialQt5.7
 Date of build:
Thu, 03 Aug 2017 10:15:14 +
 Build duration:
2 min 20 sec and counting
   JUnit Tests
  Name: (root) Failed: 3 test(s), Passed: 8 test(s), Skipped: 0 test(s), Total: 11 test(s)Failed: TestSuite.testfallbackpackage-appstreamFailed: TestSuite.testpackage-appstreamFailed: TestSuite.testpackage-nodisplay-appstream
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report100%
(5/5)96%
(22/23)96%
(22/23)73%
(1498/2064)50%
(1032/2052)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(10/10)100%
(10/10)100%
(500/502)52%
(277/532)autotests.mockdepresolver100%
(1/1)100%
(1/1)78%
(14/18)58%
(7/12)src.kpackage75%
(3/4)75%
(3/4)73%
(538/739)62%
(524/840)src.kpackage.private100%
(6/6)100%
(6/6)79%
(290/367)53%
(113/212)src.kpackagetool100%
(2/2)100%
(2/2)36%
(156/438)24%
(111/456)

build.log
Description: Binary data

D7069: kpackagetool can now write appstream data to a file

[Harald Sitter]

This revision was automatically updated to reflect the committed changes.
sitter marked an inline comment as done.
Closed by commit R290:ef15ef0889d2: kpackagetool can now write appstream data 
to a file (authored by sitter).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D7069?vs=17556=17616#toc

REPOSITORY
  R290 KPackage

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7069?vs=17556=17616

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

AFFECTED FILES
  KF5PackageMacros.cmake
  autotests/CMakeLists.txt
  autotests/data/testpackage-nodisplay/contents.hash
  autotests/data/testpackage-nodisplay/contents/images/empty.png
  autotests/data/testpackage-nodisplay/contents/ui/main.qml
  autotests/data/testpackage-nodisplay/contents/ui/otherfile.qml
  autotests/data/testpackage-nodisplay/metadata.desktop
  autotests/kpackagetoolappstreamtest.cmake
  src/kpackagetool/kpackagetool.cpp
  src/kpackagetool/main.cpp

To: sitter, apol, sebas, mart
Cc: #frameworks

D7069: kpackagetool can now write appstream data to a file

[Harald Sitter]

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

INLINE COMMENTS

> apol wrote in kpackagetool.cpp:473
> Why the `? true : false`?

lol, brain fart that.

REPOSITORY
  R290 KPackage

BRANCH
  master

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

To: sitter, apol, sebas, mart
Cc: #frameworks

D7011: Extract KPasswordLineEdit class

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> mlaurent wrote in kpasswordlineedit.cpp:30
> Otherwise it failed to using Q_PRIVATE_SLOT...

That's fishy. I just checked a single class (KColumnResizer), and it has just 
"class KColumnResizerPrivate" (no namespace, no QObject), and still has a 
Q_PRIVATE_SLOT in its KColumnResizer class.

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

To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kossebau, elvisangelaccio, #frameworks

D7011: Extract KPasswordLineEdit class

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> cfeck wrote in kpasswordlineedit.cpp:30
> Ping?

You made it a namespaced class again. Why?

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

To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kossebau, elvisangelaccio, #frameworks

D7011: Extract KPasswordLineEdit class

[Laurent Montel]

mlaurent updated this revision to Diff 17615.
mlaurent added a comment.


  Fix all comment reported

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7011?vs=17606=17615

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/knewpasswordwidgettest.cpp
  autotests/kpassworddialogautotest.cpp
  autotests/kpasswordlineedittest.cpp
  autotests/kpasswordlineedittest.h
  src/CMakeLists.txt
  src/knewpasswordwidget.cpp
  src/knewpasswordwidget.h
  src/knewpasswordwidget.ui
  src/kpassworddialog.cpp
  src/kpassworddialog.h
  src/kpassworddialog.ui
  src/kpasswordlineedit.cpp
  src/kpasswordlineedit.h
  tests/CMakeLists.txt
  tests/kpasswordlineedit_test.cpp

To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kossebau, elvisangelaccio, #frameworks

D7011: Extract KPasswordLineEdit class

[Laurent Montel]

mlaurent marked 4 inline comments as done.
mlaurent added inline comments.

INLINE COMMENTS

> cfeck wrote in knewpasswordwidget.cpp:118
> Ah, now I understand. Also, use the passed echoMode, instead of accessing 
> lineEdit().

we can't as we use it in :
connect(toggleEchoModeAction, ::triggered, this, 
::_k_echoModeToggled);

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

To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kossebau, elvisangelaccio, #frameworks

D7011: Extract KPasswordLineEdit class

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> cfeck wrote in knewpasswordwidget.cpp:75
> While you are at renaming slots: -> _k_passwordChanged

Ah wait, you are using the same slot for the verify widget, so maybe scratch 
that comment.

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

To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kossebau, elvisangelaccio, #frameworks

D7071: Fix issue where notifications will show as 1 pixel line if primary screen wasn't the leftmost one

[Matan Keren]

matank added a comment.


  Calling syncToMainItemSize() in componentComplete() seems to solve the issue 
, so I checked what is being done in syncToMainItemSize() that affects the 
issue.
  I found that resizing the dialog is what  seems to resolve the issue so I 
copied the relevant code from syncToMainItemSize() to avoid running 
updateTheme() more  than once.
  
  The only issue with what I have done that it seems like dialog size should be 
set correctly later on as it works without this code if the primary screen is 
the left most one.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: matank, #plasma, davidedmundson
Cc: plasma-devel, davidedmundson, ltoscano, #frameworks, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

D7085: Fix issue where notifications will show as 1 pixel line

[David Edmundson]

davidedmundson added a comment.


  Apparently doesn't fix the bug that brought me here.
  
  To me the code still makes sense, but equally I don't want to touch this 
clearly cursed class any more than possible.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: davidedmundson
Cc: #frameworks

D7011: Extract KPasswordLineEdit class

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in knewpasswordwidget.cpp:118
> Then please rename the slot to `_k_echoModeToggled`, otherwise is confusing.

Ah, now I understand. Also, use the passed echoMode, instead of accessing 
lineEdit().

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

To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kossebau, elvisangelaccio, #frameworks

D7011: Extract KPasswordLineEdit class

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> cfeck wrote in kpasswordlineedit.cpp:30
> Does this class need to be a QObject? If not, remove the QObject stuff.

Ping?

> kpasswordlineedit.h:53
> +Q_PROPERTY(bool clearButtonEnabled READ isClearButtonEnabled WRITE 
> setClearButtonEnabled)
> +Q_PROPERTY(QLineEdit::EchoMode echoMode READ echoMode WRITE setEchoMode)
> +public:

NOTIFY echoModeChanged

> kpasswordlineedit.h:83
> +/**
> + * Show/hide clear button
> + */

Document default value.

> kpasswordlineedit.h:93
> +/**
> + * Change echo mode
> + */

Please document the default value. If it is indeed QLineEdit::Password (which I 
would expect), remove the properties from the .ui files.

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

To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kossebau, elvisangelaccio, #frameworks

D7011: Extract KPasswordLineEdit class

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> mlaurent wrote in knewpasswordwidget.cpp:118
> Nope it's ok as code is called after changing echomode
> I created an autotest for testing it

Then please rename the slot to `_k_echoModeToggled`, otherwise is confusing.

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

To: mlaurent, cfeck, dfaure, elvisangelaccio
Cc: kossebau, elvisangelaccio, #frameworks

D7071: Fix issue where notifications will show as 1 pixel line if primary screen wasn't the leftmost one

[David Edmundson]

davidedmundson added a comment.


  Can you explain the logic behind adding the code you've added.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: matank, #plasma, davidedmundson
Cc: plasma-devel, davidedmundson, ltoscano, #frameworks, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

D6197: Add kauth helper to file ioslave

[David Faure]

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

INLINE COMMENTS

> chinmoyr wrote in filehelper.cpp:86
> In case of OPEN and OPENDIR, I can see why we need error checking, opendir() 
> can return null and there are some issues (I read on internet, can't recall 
> the source) if close() fails. But in case of single function calls like 
> chmod() I can't see how return value matters. Say if unlink fails for 
> whatever reasons, we can return the error code and the checking for exact 
> error code can be done by ioslave. Is there something I am overlooking?

The documentation for chown and others says:

Upon successful completion, these functions shall return 0.  Otherwise, these 
functions shall return −1 and set errno to indicate the error.

It does NOT say, that errno isn't set when the function is successful. It seems 
to me that it would be perfectly valid for a libc implementation to do 
something like "try this, it fails (and sets errno), then try that, it worked, 
return 0".

For this reason I would feel much safer (especially in code run by root!) if 
the error handling was more classic, i.e. by checking return values.

> filehelper.cpp:41
> +
> +enum {
> +CHMOD = 1,

I assume this enum is duplicated elsewhere (in whoever is serializing the 
arguments for FileHelper), right?
Can the duplication be avoided by sharing a private header? Otherwise this at 
least needs a comment like "keep in sync with..." in both places.

> filehelper.cpp:102
> +closedir(dp);
> +}
> +sendFileDescriptor(-1);

missing else or break here, no?
You're sending the valid fd, followed by -1.

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

To: chinmoyr, elvisangelaccio, #frameworks, dfaure

KDE CI: Frameworks bluez-qt kf5-qt5 XenialQt5.7 - Build # 16 - Still Unstable!

[no-reply]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20bluez-qt%20kf5-qt5%20XenialQt5.7/16/
 Project:
Frameworks bluez-qt kf5-qt5 XenialQt5.7
 Date of build:
Thu, 03 Aug 2017 07:11:40 +
 Build duration:
2 min 58 sec and counting
   JUnit Tests
  Name: (root) Failed: 8 test(s), Passed: 2 test(s), Skipped: 0 test(s), Total: 10 test(s)Failed: TestSuite.bluezqt-adaptertestFailed: TestSuite.bluezqt-agentmanagertestFailed: TestSuite.bluezqt-devicetestFailed: TestSuite.bluezqt-inputtestFailed: TestSuite.bluezqt-managertestFailed: TestSuite.bluezqt-mediaplayertestFailed: TestSuite.bluezqt-obexmanagertestFailed: TestSuite.bluezqt-qmltests
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report100%
(3/3)48%
(41/86)48%
(41/86)25%
(1030/4146)7%
(312/4230)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(18/18)100%
(18/18)49%
(684/1383)8%
(235/2902)src35%
(19/54)35%
(19/54)14%
(315/2331)6%
(72/1270)src.imports29%
(4/14)29%
(4/14)7%
(31/432)9%
(5/58)

build.log
Description: Binary data

D7071: Fix issue where notifications will show as 1 pixel line if primary screen wasn't the leftmost one

[Matan Keren]

matank updated this revision to Diff 17609.
matank added a comment.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.


  - Fix issue where notifications will show as 1 pixel line
  
  Sets the dialog to his correct size to prevent it showing as 1 pixel line, 
  still a bit hackish not sure why the dialog size can be set incorrectly

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7071?vs=17559=17609

BRANCH
  master

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

AFFECTED FILES
  src/plasmaquick/dialog.cpp

To: matank, #plasma, davidedmundson
Cc: plasma-devel, davidedmundson, ltoscano, #frameworks, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas