Re: Re: How to port KIntSpinBox::setSuffix

[Martin Gräßlin]

On Tuesday 03 June 2014 17:59:18 David Faure wrote:
> PS: Martin: meanwhile Laurent committed the class (which I reviewed) to
> ktextwidgets, you can use that.

Laurent CC-ed me on the commit and I already adjusted the code to use it. 
Thanks a lot for the fast fix.

Cheers
Martin


signature.asc
Description: This is a digitally signed message part.
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Is plasma-framework not in tier 3?

[Aurélien Gâteau]

On Tue, Jun 3, 2014, at 17:04, Marko Käning wrote:
> I was wondering why I couldn't find plasma-framework in tier 3 on
> api.kde.org not below frameworks [1] although it is actually shown as
> part of tier 3 in the dependency graph in [2] ...

The name on the landing page are based on the CMake project name: this
cause plasma-framework to be listed as "Plasma" there.

Aurélien
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118458: Fix KDirWatch to act more reliably against various backends.

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118458/#review59123
---


Very good to see that it brings the FAM backend to be more in line with the 
others, behavior-wise.

Just a few small issues.


autotests/kdirwatch_unittest.cpp


It seems that the second call should pass "created(QString)" rather than 
"deleted(QString)".



autotests/kdirwatch_unittest.cpp


The commit log doesn't mention any changes related to QFileSystemWatcher, 
only to the FAM backend. Are you sure you wanted to commit this? If it works 
then of course I'm happy with it (but maybe in a separate commit then)



autotests/kdirwatch_unittest.cpp


This could actually use QFile::link(), would be more readable.



src/lib/io/kdirwatch.cpp


I'm not 100% sure about the meaning of this boolean variable. (we started 
an event?)
Maybe it can be called something more explicit (waitForInitialFAMEvents? or 
something that makes sense to you ;))



src/lib/io/kdirwatch.cpp


Technically this could be an event about a completely unrelated file to the 
one we're waiting to see, couldn't it?




src/lib/io/kdirwatch.cpp


For performance reasons it was good for this check to be as early as 
possible. But of course the risk is an infinite loop if we wait to "see" such a 
file and we never see it. The solution could be to ensure that we never start 
watching such a file in the first place...
Anyhow, not a blocker.



src/lib/io/kdirwatch.cpp


actual -> actually ?


- David Faure


On June 1, 2014, 9:03 p.m., Matthew Dawson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118458/
> ---
> 
> (Updated June 1, 2014, 9:03 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Bugs: 329802
> https://bugs.kde.org/show_bug.cgi?id=329802
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Fix KDirWatch to act more reliably against various backends.
> 
>  - When KDirWatch used a FAM backend, it wouldn't actually wait for the
> necessary watches to be put in place before returning, allowing for a race
> where an application may think a watch is in place when it really isn't.
> This was easily seen using gamin and running the testDeleteAndRecreateFile
> test.  Fix by delaying useFAM until the watch is in place by waiting until
> the EndExist FAM event is received.  This adds ~100-200ms per watch with
> gamin.
>  - When a file is deleted and recreated, and scanEntry detects it, the dirty
> signal would be emitted.  Fix to emit a deleted + created signal, as expected.
>  - When a deleted signal was requested, other signals were dropped.  Due to
> the above point, this would stop the created signal from being emitted.  Now,
> all signals are emitted, even when a delete is detected.
>  - When watching recreated files, the created signal could get lost as there
> was a race between when the created signal would be generated and its signal
> spy would be created.  Fix by making sure the spy is created before the signal
> could be emitted.
>  - Make sure the created signal isn't emitted twice with both FAM and the
> polling timer.  This occurs as FAM doesn't fix up the fact the entry has been
> created, and the poller thus assumes it needs to notify the world.  Fix by
> having FAM not emit the event in this case.
> 
> BUG: 329802
> 
> Note that due to a bug in the FAM daemon (which doesn't exist in the gamin 
> daemon), the testDeleteAndRecreateDir unit test fails as FAM doesn't detect 
> the folder deletion.  This test previously passed due to the race condition 
> outlined in bug 329802, which was fixed in this RR.  I've confirmed this is a 
> bug in FAM using a minimal program talking directly to the daemon, and 
> watching the strace output from FAM.  FAM doesn't detect when a folder it is 
> watching directly is deleted.  It does detect when a folder inside a folder 
> it is watching is deleted, however.
> I think this should go in as is, as the unit test failing exposes the actual 
> problem, instead of hiding behind a race when it doesn't really work.  If 
> running against FAM is important, I can see about tracking down the issue.
> 
> 
> Diffs
> -
> 
>   autotests/kdirwatch_unittest.cpp 4e638e089012276dc8b17b9d960732a161cc3be1 
>   src/lib/io/kdirwatch.cpp cc6feec4d2b2598e

Re: Review Request 118474: Move networkstatus kded module to kf5/kded subdirectory

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118474/#review59125
---

Ship it!


Ship It!

- David Faure


On June 2, 2014, 4:20 p.m., Alex Merry wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118474/
> ---
> 
> (Updated June 2, 2014, 4:20 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs4support
> 
> 
> Description
> ---
> 
> Move networkstatus kded module to kf5/kded subdirectory
> 
> This makes sure the installed file is properly versioned.
> 
> 
> Diffs
> -
> 
>   src/solid-networkstatus/kded/CMakeLists.txt 
> ca6bef21d4a8450651eaa8326898b6bfecdbb51a 
>   src/solid-networkstatus/kded/networkstatus.desktop 
> b792b3cbe1b4dc95e74426ca358ea7005f8c7998 
> 
> Diff: https://git.reviewboard.kde.org/r/118474/diff/
> 
> 
> Testing
> ---
> 
> Module is properly loaded when accessing the 
> org.kde.kded5/modules/networkstatus D-Bus interface.
> 
> 
> Thanks,
> 
> Alex Merry
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118476: Move kded module to kf5/kded subdirectory

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118476/#review59126
---

Ship it!


Ship It!

- David Faure


On June 2, 2014, 4:39 p.m., Alex Merry wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118476/
> ---
> 
> (Updated June 2, 2014, 4:39 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Move kded module to kf5/kded subdirectory
> 
> This makes sure the module is properly versioned.
> 
> 
> Diffs
> -
> 
>   src/platformstatus/CMakeLists.txt 676a8be30df41b042758d55b58caa22fc6c89311 
>   src/platformstatus/kded_platformstatus.desktop 
> 9976fa3041a6c12fc4b3ce2146fb481181fa65b8 
> 
> Diff: https://git.reviewboard.kde.org/r/118476/diff/
> 
> 
> Testing
> ---
> 
> qdbus org.kde.kded5 /kded loadModule kded_platformstatus
> succeeds
> 
> 
> Thanks,
> 
> Alex Merry
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118486: Put transcript plugin in kf5 subdir and simplify search logic

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118486/#review59127
---

Ship it!


Ship It!

- David Faure


On June 2, 2014, 10:03 p.m., Alex Merry wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118486/
> ---
> 
> (Updated June 2, 2014, 10:03 p.m.)
> 
> 
> Review request for KDE Frameworks and Chusslove Illich.
> 
> 
> Repository: ki18n
> 
> 
> Description
> ---
> 
> Put transcript plugin in kf5 subdir and simplify search logic
> 
> Putting the plugin in a versioned subdirectory ensures there will not be
> any coinstallability issues with KF6 if it is Qt5-based. Using
> QPluginLoader to find the plugin makes for simpler, more reliable code.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 3099d2d29e43e40ac761694019f06aec41e54147 
>   src/klocalizedstring.cpp 4a4f6f8c1a79d4b101ae3df5e589cb629f4bd596 
> 
> Diff: https://git.reviewboard.kde.org/r/118486/diff/
> 
> 
> Testing
> ---
> 
> Tests pass, but not sure how to test the installed location. Chusslove: do 
> you have a standard way to test the loading and operation of the transcript 
> plugin?
> 
> 
> Thanks,
> 
> Alex Merry
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Is plasma-framework not in tier 3?

[Marko Käning]

Hi Aurélien,

On 04 Jun 2014, at 09:15 , Aurélien Gâteau  wrote:
> The name on the landing page are based on the CMake project name: this
> cause plasma-framework to be listed as "Plasma" there.

but why does plasma and plasma-framework appear as separate frameworks in the 
dependency graph [1]?
Is that a glitch of the script which creates the dot graphs then?

Greets,
Marko




[1] 
http://api.kde.org/frameworks-api/frameworks5-apidocs/plasma-framework/html/plasma-framework-dependencies.html
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

(UNINTENDED?) installation outside the configured DATA_INSTALL_DIR (was Issues for Qt5/KF5/KDE/CI system on OSX/MacPorts)

[Marko Käning]

It turns out that there are indeed a few more KF5 framework trying to install
their files in “/Library/Application Support”, here’s the full list of kf5 
folders
found in the install environment:
---
./frameworkintegration/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/frameworkintegration/inst/Library/Application
 Support/kf5
./kconfigwidgets/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/kconfigwidgets/inst/Library/Application
 Support/kf5
./kdelibs4support/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/kdelibs4support/inst/Library/Application
 Support/kf5
./kdesignerplugin/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/kdesignerplugin/inst/Library/Application
 Support/kf5
./kdoctools/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/kdoctools/inst/Library/Application
 Support/kf5
./khtml/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/khtml/inst/Library/Application
 Support/kf5
./kio/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/kio/inst/Library/Application
 Support/kf5
./knewstuff/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/knewstuff/inst/Library/Application
 Support/kf5
./kxmlgui/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/kxmlgui/inst/Library/Application
 Support/kf5
---

Turns out thought that these aren’t all yet, since two frameworks (khtml & 
ktexteditor)
actually place their stuff outside of the actually configured kf5 subdirectory:
---
$ find . -type d -wholename "./*/local-inst/opt/kde/*/Library/Application 
Support/*" | grep -v "/kf5/" | egrep -v "/kf5$"
./khtml/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/khtml/inst/Library/Application
 Support/khtml
./ktexteditor/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/ktexteditor/inst/Library/Application
 Support/katepart
./ktexteditor/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/ktexteditor/inst/Library/Application
 Support/katepart5
./ktexteditor/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/ktexteditor/inst/Library/Application
 Support/katepart5/script
./ktexteditor/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/ktexteditor/inst/Library/Application
 Support/katepart5/script/commands
./ktexteditor/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/ktexteditor/inst/Library/Application
 Support/katepart5/script/files
./ktexteditor/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/ktexteditor/inst/Library/Application
 Support/katepart5/script/files/quickcoding
./ktexteditor/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/ktexteditor/inst/Library/Application
 Support/katepart5/script/files/quickcoding/cpp
./ktexteditor/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/ktexteditor/inst/Library/Application
 Support/katepart5/script/indentation
./ktexteditor/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/ktexteditor/inst/Library/Application
 Support/katepart5/script/libraries
./ktexteditor/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/ktexteditor/inst/Library/Application
 Support/katepart5/script/libraries/emmet
./ktexteditor/local-inst/opt/kde/install/darwin/mavericks/clang/kf5-qt5/frameworks/ktexteditor/inst/Library/Application
 Support/katepart5/syntax
---
which is probably unintended! Can this be considered a glitch of their CMake 
files?


Greets,
Marko
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Is plasma-framework not in tier 3?

[Aurélien Gâteau]

On Wed, Jun 4, 2014, at 1:11, Marko Käning wrote:
> Hi Aurélien,
> 
> On 04 Jun 2014, at 09:15 , Aurélien Gâteau  wrote:
> > The name on the landing page are based on the CMake project name: this
> > cause plasma-framework to be listed as "Plasma" there.
> 
> but why does plasma and plasma-framework appear as separate frameworks in
> the dependency graph [1]?
> Is that a glitch of the script which creates the dot graphs then?

That's definitely a bug. Going to investigate. Thanks for pointing it
out.

Aurélien
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118269: Fix crash when showing the confirmation dialog for trash/delete operations

[Frank Reininghaus]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118269/
---

(Updated June 4, 2014, 8:58 a.m.)


Status
--

This change has been discarded.


Review request for KDE Frameworks and David Faure.


Bugs: 334648
https://bugs.kde.org/show_bug.cgi?id=334648


Repository: kjobwidgets


Description
---

Currently Dolphin (and probably anything else that can delete files using KIO) 
crashes when it's supposed to show the confirmation dialog.

The problem is that KonqOperations::askDeleteConfirmation() sets up a 
KIO::JobUiDelegate object which has no associated job, and calls its 
setWindow(QWidget*) method before calling the actual askDeleteConfirmation() 
function.

KIO::JobUiDelegate::setWindow(QWidget*) then calls the setWindow function of 
the base class, KDialogJobUiDelegate::setWindow(QWidget*), which then crashes 
because it contains the line

Q_ASSERT(job())

and there is no job.

The problem does not exist in KDE SC 4.x - the code went through a big 
refactoring in

https://git.reviewboard.kde.org/r/111081/

which added the assert.

Just removing the assert won't help because the function then calls
KJobWidgets::setWindow(KJob *job, QWidget *widget)
which dereferences the job, i.e., we get a segfault instead.

This patch removes the assert and wraps the function in an "if (job())" block 
instead. I'm not entirely sure if that is the correct solution though - any 
feedback is welcome. Alternatively, one could move the if-check to the child 
class, i.e., to KIO::JobUiDelegate::setWindow(QWidget*), if this is the only 
valid use of a KDialogJobUiDelegate without a job.

Or maybe it does not make much sense at all to have the askDeleteConfirmation 
function, which is probably always called before any job is set up, in a 
KDialogJobUiDelegate subclass? Changing that would probably require more 
intrusive changes though.


Diffs
-

  src/kdialogjobuidelegate.cpp fb4c99a 

Diff: https://git.reviewboard.kde.org/r/118269/diff/


Testing
---

Fixes the crash for me, and the confirmation dialog works as expected (i.e., 
the user can choose if the file should really be deleted/trashed or not).


Thanks,

Frank Reininghaus

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118269: Fix crash when showing the confirmation dialog for trash/delete operations

[Frank Reininghaus]

> On June 1, 2014, 3:01 p.m., David Faure wrote:
> > That makes the setWindow call (as called by konq_operations.cpp) useless, 
> > though.
> > 
> > This is related to the line in jobuidelegate.cpp which says
> >QWidget *widget = job() ? window() : NULL; // ### job is NULL here, most 
> > of the time, right?
> > 
> > Clearly we need a better way to pass the QWidget* parent to the 
> > askDeleteConfirmation() method, it's not working on either end (because no 
> > job).
> > 
> > But the API (JobUiDelegateExtension) is in KIOCore, so no QWindow nor 
> > QWidget there.
> > 
> > I think we need to use a member variable in KDialogJobUiDelegate in 
> > addition to setting it in KJobWidgets.
> 
> David Faure wrote:
> http://www.davidfaure.fr/2014/kjobwidgets.diff
> http://www.davidfaure.fr/2014/kio_jobuidelegate.diff
> 
> Seems to work, I get a widget pointer when confirmation dialog pops up.

Thanks David!


- Frank


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118269/#review58902
---


On May 22, 2014, 9:39 p.m., Frank Reininghaus wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118269/
> ---
> 
> (Updated May 22, 2014, 9:39 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 334648
> https://bugs.kde.org/show_bug.cgi?id=334648
> 
> 
> Repository: kjobwidgets
> 
> 
> Description
> ---
> 
> Currently Dolphin (and probably anything else that can delete files using 
> KIO) crashes when it's supposed to show the confirmation dialog.
> 
> The problem is that KonqOperations::askDeleteConfirmation() sets up a 
> KIO::JobUiDelegate object which has no associated job, and calls its 
> setWindow(QWidget*) method before calling the actual askDeleteConfirmation() 
> function.
> 
> KIO::JobUiDelegate::setWindow(QWidget*) then calls the setWindow function of 
> the base class, KDialogJobUiDelegate::setWindow(QWidget*), which then crashes 
> because it contains the line
> 
> Q_ASSERT(job())
> 
> and there is no job.
> 
> The problem does not exist in KDE SC 4.x - the code went through a big 
> refactoring in
> 
> https://git.reviewboard.kde.org/r/111081/
> 
> which added the assert.
> 
> Just removing the assert won't help because the function then calls
> KJobWidgets::setWindow(KJob *job, QWidget *widget)
> which dereferences the job, i.e., we get a segfault instead.
> 
> This patch removes the assert and wraps the function in an "if (job())" block 
> instead. I'm not entirely sure if that is the correct solution though - any 
> feedback is welcome. Alternatively, one could move the if-check to the child 
> class, i.e., to KIO::JobUiDelegate::setWindow(QWidget*), if this is the only 
> valid use of a KDialogJobUiDelegate without a job.
> 
> Or maybe it does not make much sense at all to have the askDeleteConfirmation 
> function, which is probably always called before any job is set up, in a 
> KDialogJobUiDelegate subclass? Changing that would probably require more 
> intrusive changes though.
> 
> 
> Diffs
> -
> 
>   src/kdialogjobuidelegate.cpp fb4c99a 
> 
> Diff: https://git.reviewboard.kde.org/r/118269/diff/
> 
> 
> Testing
> ---
> 
> Fixes the crash for me, and the confirmation dialog works as expected (i.e., 
> the user can choose if the file should really be deleted/trashed or not).
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118474: Move networkstatus kded module to kf5/kded subdirectory

[Commit Hook]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118474/#review59136
---


This review has been submitted with commit 
eb51d7f228d8b4b611ca36df35a8618afd5a290c by Alex Merry to branch master.

- Commit Hook


On June 2, 2014, 4:20 p.m., Alex Merry wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118474/
> ---
> 
> (Updated June 2, 2014, 4:20 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs4support
> 
> 
> Description
> ---
> 
> Move networkstatus kded module to kf5/kded subdirectory
> 
> This makes sure the installed file is properly versioned.
> 
> 
> Diffs
> -
> 
>   src/solid-networkstatus/kded/CMakeLists.txt 
> ca6bef21d4a8450651eaa8326898b6bfecdbb51a 
>   src/solid-networkstatus/kded/networkstatus.desktop 
> b792b3cbe1b4dc95e74426ca358ea7005f8c7998 
> 
> Diff: https://git.reviewboard.kde.org/r/118474/diff/
> 
> 
> Testing
> ---
> 
> Module is properly loaded when accessing the 
> org.kde.kded5/modules/networkstatus D-Bus interface.
> 
> 
> Thanks,
> 
> Alex Merry
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118474: Move networkstatus kded module to kf5/kded subdirectory

[Alex Merry]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118474/
---

(Updated June 4, 2014, 9:15 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Repository: kdelibs4support


Description
---

Move networkstatus kded module to kf5/kded subdirectory

This makes sure the installed file is properly versioned.


Diffs
-

  src/solid-networkstatus/kded/CMakeLists.txt 
ca6bef21d4a8450651eaa8326898b6bfecdbb51a 
  src/solid-networkstatus/kded/networkstatus.desktop 
b792b3cbe1b4dc95e74426ca358ea7005f8c7998 

Diff: https://git.reviewboard.kde.org/r/118474/diff/


Testing
---

Module is properly loaded when accessing the 
org.kde.kded5/modules/networkstatus D-Bus interface.


Thanks,

Alex Merry

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118476: Move kded module to kf5/kded subdirectory

[Commit Hook]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118476/#review59137
---


This review has been submitted with commit 
1a8032610353a592831863774e6e1abc4b6f5189 by Alex Merry to branch master.

- Commit Hook


On June 2, 2014, 4:39 p.m., Alex Merry wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118476/
> ---
> 
> (Updated June 2, 2014, 4:39 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Move kded module to kf5/kded subdirectory
> 
> This makes sure the module is properly versioned.
> 
> 
> Diffs
> -
> 
>   src/platformstatus/CMakeLists.txt 676a8be30df41b042758d55b58caa22fc6c89311 
>   src/platformstatus/kded_platformstatus.desktop 
> 9976fa3041a6c12fc4b3ce2146fb481181fa65b8 
> 
> Diff: https://git.reviewboard.kde.org/r/118476/diff/
> 
> 
> Testing
> ---
> 
> qdbus org.kde.kded5 /kded loadModule kded_platformstatus
> succeeds
> 
> 
> Thanks,
> 
> Alex Merry
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118476: Move kded module to kf5/kded subdirectory

[Alex Merry]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118476/
---

(Updated June 4, 2014, 9:16 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Plasma.


Repository: plasma-framework


Description
---

Move kded module to kf5/kded subdirectory

This makes sure the module is properly versioned.


Diffs
-

  src/platformstatus/CMakeLists.txt 676a8be30df41b042758d55b58caa22fc6c89311 
  src/platformstatus/kded_platformstatus.desktop 
9976fa3041a6c12fc4b3ce2146fb481181fa65b8 

Diff: https://git.reviewboard.kde.org/r/118476/diff/


Testing
---

qdbus org.kde.kded5 /kded loadModule kded_platformstatus
succeeds


Thanks,

Alex Merry

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118486: Put transcript plugin in kf5 subdir and simplify search logic

[Alex Merry]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118486/
---

(Updated June 4, 2014, 9:18 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Chusslove Illich.


Repository: ki18n


Description
---

Put transcript plugin in kf5 subdir and simplify search logic

Putting the plugin in a versioned subdirectory ensures there will not be
any coinstallability issues with KF6 if it is Qt5-based. Using
QPluginLoader to find the plugin makes for simpler, more reliable code.


Diffs
-

  src/CMakeLists.txt 3099d2d29e43e40ac761694019f06aec41e54147 
  src/klocalizedstring.cpp 4a4f6f8c1a79d4b101ae3df5e589cb629f4bd596 

Diff: https://git.reviewboard.kde.org/r/118486/diff/


Testing
---

Tests pass, but not sure how to test the installed location. Chusslove: do you 
have a standard way to test the loading and operation of the transcript plugin?


Thanks,

Alex Merry

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118486: Put transcript plugin in kf5 subdir and simplify search logic

[Commit Hook]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118486/#review59138
---


This review has been submitted with commit 
8bb821a959fa71c405a1e94bc73aa20920254122 by Alex Merry to branch master.

- Commit Hook


On June 2, 2014, 10:03 p.m., Alex Merry wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118486/
> ---
> 
> (Updated June 2, 2014, 10:03 p.m.)
> 
> 
> Review request for KDE Frameworks and Chusslove Illich.
> 
> 
> Repository: ki18n
> 
> 
> Description
> ---
> 
> Put transcript plugin in kf5 subdir and simplify search logic
> 
> Putting the plugin in a versioned subdirectory ensures there will not be
> any coinstallability issues with KF6 if it is Qt5-based. Using
> QPluginLoader to find the plugin makes for simpler, more reliable code.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 3099d2d29e43e40ac761694019f06aec41e54147 
>   src/klocalizedstring.cpp 4a4f6f8c1a79d4b101ae3df5e589cb629f4bd596 
> 
> Diff: https://git.reviewboard.kde.org/r/118486/diff/
> 
> 
> Testing
> ---
> 
> Tests pass, but not sure how to test the installed location. Chusslove: do 
> you have a standard way to test the loading and operation of the transcript 
> plugin?
> 
> 
> Thanks,
> 
> Alex Merry
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118490: Change all occurrences of KDE_NO_DEPRECATED to an appropriate define.

[Alex Merry]

> On June 3, 2014, 10:16 a.m., Alex Merry wrote:
> > src/mdnsd-servicetypebrowser.cpp, line 51
> > 
> >
> > Interesting... any idea which monday?
> 
> Matthew Dawson wrote:
> I'm assuming that is any Monday, as that used to be the day to break 
> SIC/BIC on kdelibs.  It was there when I inherited the code.  I'll just 
> remove the comment in an updated patch.

Ah, yes, I recall now.


> On June 3, 2014, 10:16 a.m., Alex Merry wrote:
> > src/servicetypebrowser.h, line 93
> > 
> >
> > Since when?
> 
> Matthew Dawson wrote:
> At least since KDELibs 4 time.  Interestingly, Git shows the @deprecated 
> came in 2008, from a commit you made :).  Looking back further, it seems to 
> just have been considered unnecessary and should be removed as of 2007, and 
> for the 4.4 releases effectively.  I'll update the comment if the function is 
> kept.

Heh, so the sparse message is my fault :-)


> On June 3, 2014, 10:16 a.m., Alex Merry wrote:
> > src/servicetypebrowser.h, lines 97-99
> > 
> >
> > It looks like the mdnsd backend even just returns false, so this is not 
> > even reliable. With that in mind, I'd be tempted to make it an inline 
> > method that just always returns false.
> 
> Matthew Dawson wrote:
> Is it still late to do a minor SIC change?  It appears the function is 
> useless, and is uneeded.  Its only use seems to be for a structure similar to:
> if (!isRunning()) {
>   startBrowse();
> }
> which can just be reduced to:
> startBrowse();
> as startBrowse can be called multiple times.
> 
> If it is preferred to keep it for SC purposes, then I'll take your 
> suggestion and just make it inline and return false.

I'd say at this point (after the last beta), and given the negligible cost of 
keeping it, it should stay.


- Alex


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118490/#review59066
---


On June 3, 2014, 6:59 a.m., Matthew Dawson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118490/
> ---
> 
> (Updated June 3, 2014, 6:59 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdnssd
> 
> 
> Description
> ---
> 
> Change all occurrences of KDE_NO_DEPRECATED to an appropriate define.
> 
> Inline with the new defines used by Frameworks, remove the usage of the
> KDE_NO_DEPRECATED define.
> 
> 
> Diffs
> -
> 
>   src/avahi-servicetypebrowser.cpp b3c14f84367d5093a6cee5ef3f684edc201a3f96 
>   src/dummy-servicetypebrowser.cpp 39a4b6b660bb47b68b5721535994487dbe2abbf6 
>   src/mdnsd-servicetypebrowser.cpp d16dd1fa1c692cd20e21f1a7d5471ac7956469b3 
>   src/servicetypebrowser.h 3cd10e36bf089e088e8a48bb0cf73daa4fb859c5 
> 
> Diff: https://git.reviewboard.kde.org/r/118490/diff/
> 
> 
> Testing
> ---
> 
> Code still compiles.
> 
> 
> Thanks,
> 
> Matthew Dawson
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 118514: Add KColumnResizer to KWidgetsAddons

[Aurélien Gâteau]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118514/
---

Review request for KDE Frameworks and Christoph Feck.


Repository: kwidgetsaddons


Description
---

I have been asked to add ColumnResizer to KWidgetAddons. ColumnResizer is a 
helper class I created long ago to help maintain uniform widths across layouts. 
You can learn more about it from the blog post I wrote back then: 
http://agateau.com/2011/clean-up-your-layouts-with-columnresizer/ . I 
simplified the API a bit, added some documentation, and here it is.


Diffs
-

  src/CMakeLists.txt 27b9084 
  src/kcolumnresizer.h PRE-CREATION 
  src/kcolumnresizer.cpp PRE-CREATION 
  tests/CMakeLists.txt eccf887 
  tests/kcolumnresizertest.h PRE-CREATION 
  tests/kcolumnresizertest.cpp PRE-CREATION 
  tests/kcolumnresizertest.ui PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/118514/diff/


Testing
---

Manual test program behaves as expected.


Thanks,

Aurélien Gâteau

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118489: Change all occurrences of KDE_NO_DEPRECATED to an appropriate define.

[Alex Merry]

> On June 3, 2014, 10:12 a.m., Alex Merry wrote:
> > Changing the macro name is a no-brainer. Comments below; a lot of them are 
> > about binary compatibility issues, and these depend on how we expect this 
> > macro to be used. I believe that Qt5 makes all its deprecated functions 
> > header-only so that compiling Qt without deprecated symbols produces a 
> > version BC with the version with deprecated symbols, and you can also 
> > safely set the macro yourself in application code to hide all Qt's 
> > deprecated symbols without messing anything up.
> > 
> > As you've currently got it, the only way this macro is useful is to produce 
> > a separate, binary-incompatible version of the framework that has no 
> > deprecated code or symbols. And maybe that's the only thing it's worth 
> > supporting.
> > 
> > I also highlighted some of the @deprecated apidox things - they really need 
> > to be more helpful, and this seemed to be an "all the deprecated stuff" 
> > review.
> 
> Matthew Dawson wrote:
> Regarding the binary compatibility, kdelibs4support does a similar thing, 
> which is why I thought it might be appropriate, but I also published the RR 
> to be sure :) .  Keeping with my don't rock the boat policy, I'll take 
> whatever the Frameworks community deems to be appropriate.  We should decide 
> on a policy for deprecation in frameworks, and then publish that somewhere (I 
> may have a new volunteer that would even be willing to write it!).
> 
> Regarding the @deprecated apidox, I'll try to write something useful for 
> them and update the patch later tonight.  I won't comment on each one 
> invidivually.  Do you happen to have a good example of a @deprecated comment?

Yeah, I think that policy thing is a good idea. If you have a volunteer for 
that, that's great! The most productive approach is probably to draft something 
and send it to the list for feedback.

Example:
@deprecated since 5.0, use SomeClass::someMethod() instead


> On June 3, 2014, 10:12 a.m., Alex Merry wrote:
> > src/core/kcoreconfigskeleton.h, lines 1211-1220
> > 
> >
> > This wrapping of the apidox in the deprecated macro has been done 
> > inconsistently.
> 
> Matthew Dawson wrote:
> Is there a standard for how Frameworks code should wrap the declarations? 
>  I'll fix any to be consistent with the wider codebase.

I don't think there's a standard, no. However, I think it makes sense to wrap 
the docs - that way, it should be possible to tell doxygen to assume 
KCONFIGCORE_NO_DEPRECATED is set, and it won't get confused by dox that aren't 
attached to a declaration.


> On June 3, 2014, 10:12 a.m., Alex Merry wrote:
> > src/core/kcoreconfigskeleton.h, lines 1475-1487
> > 
> >
> > I think this is a bad idea. Binary compatibility is very important in 
> > Frameworks, and this breaks it. And messing with virtuals is not just BIC, 
> > it's BIC in a way that can cause weird error messages (rather than just 
> > linking failures).
> 
> Matthew Dawson wrote:
> As mentioned above, I wasn't sure what the policy should be.  I'm happy 
> doing it either way, but I think the wider community should decide.

Can I suggest sending a separate mail to the list? People tend to brush over 
RRs, especially ones someone else has already commented on.


- Alex


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118489/#review59060
---


On June 3, 2014, 6:34 a.m., Matthew Dawson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118489/
> ---
> 
> (Updated June 3, 2014, 6:34 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Change all occurrences of KDE_NO_DEPRECATED to an appropriate define.
> 
> Inline with the new defines used by Frameworks, remove the KDE_NO_DEPRECATED.
> 
> Also remove some deprecated virtuals when compiling without deprecated 
> features.
> 
> Make sure there are no deprecated features left when deprecated features are
> compiled out.
> 
> When compiling without deprecated features, make sure not to use them.
> 
> KEmailSettings was using deprecated entries when the enumerations for the
> features were compiled out, which would fail to compile.  Thus, when compiling
> without deprecated features don't read extra configuration entries.
> 
> Remove usages of deprecated virtuals, instead use their replacement.
> 
> Go through and rename uses of usrWriteConfig to usrSave.  The change should
> invoke no behavioural differences.  Note that the kconfig

Re: Review Request 118514: Add KColumnResizer to KWidgetsAddons

[Aurélien Gâteau]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118514/
---

(Updated June 4, 2014, 11:52 a.m.)


Review request for KDE Frameworks and Christoph Feck.


Changes
---

Fix small issues pointed out by Laurent.


Repository: kwidgetsaddons


Description
---

I have been asked to add ColumnResizer to KWidgetAddons. ColumnResizer is a 
helper class I created long ago to help maintain uniform widths across layouts. 
You can learn more about it from the blog post I wrote back then: 
http://agateau.com/2011/clean-up-your-layouts-with-columnresizer/ . I 
simplified the API a bit, added some documentation, and here it is.


Diffs (updated)
-

  src/CMakeLists.txt 27b9084 
  src/kcolumnresizer.h PRE-CREATION 
  src/kcolumnresizer.cpp PRE-CREATION 
  tests/CMakeLists.txt eccf887 
  tests/kcolumnresizertest.h PRE-CREATION 
  tests/kcolumnresizertest.cpp PRE-CREATION 
  tests/kcolumnresizertest.ui PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/118514/diff/


Testing
---

Manual test program behaves as expected.


Thanks,

Aurélien Gâteau

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118514: Add KColumnResizer to KWidgetsAddons

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118514/#review59145
---


+++




src/kcolumnresizer.h


We probably need a remove function.





src/kcolumnresizer.cpp


Do we really want it to be critical?

Sometimes widgets are loaded dynamically, we probably don't want to explode 
because some widgets might not line up.

Maybe a qWarning + a Q_ASSERT.



- David Edmundson


On June 4, 2014, 9:52 a.m., Aurélien Gâteau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118514/
> ---
> 
> (Updated June 4, 2014, 9:52 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> I have been asked to add ColumnResizer to KWidgetAddons. ColumnResizer is a 
> helper class I created long ago to help maintain uniform widths across 
> layouts. You can learn more about it from the blog post I wrote back then: 
> http://agateau.com/2011/clean-up-your-layouts-with-columnresizer/ . I 
> simplified the API a bit, added some documentation, and here it is.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 27b9084 
>   src/kcolumnresizer.h PRE-CREATION 
>   src/kcolumnresizer.cpp PRE-CREATION 
>   tests/CMakeLists.txt eccf887 
>   tests/kcolumnresizertest.h PRE-CREATION 
>   tests/kcolumnresizertest.cpp PRE-CREATION 
>   tests/kcolumnresizertest.ui PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/118514/diff/
> 
> 
> Testing
> ---
> 
> Manual test program behaves as expected.
> 
> 
> Thanks,
> 
> Aurélien Gâteau
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Jenkins build became unstable: ktexteditor_master_qt5 #416

[KDE CI System]

See 

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118514: Add KColumnResizer to KWidgetsAddons

[Dominik Haumann]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118514/#review59170
---


What about adding something like this to Qt ?


src/kcolumnresizer.h


Reading your blog 
http://agateau.com/2011/clean-up-your-layouts-with-columnresizer/
I have the impression that the documentation added here is less helpful 
than your blog.

That said, can you add one of the example images? Have a look at 
kmessagewidget.h how to do this.



src/kcolumnresizer.h


Doxygen comments without colon:
@param layout The layout containing the widgets to add 

Maybe add, tat the layout must be a QGridLayout or a QFormLayout.



src/kcolumnresizer.cpp


The API should probably still be more safe in release mode. What happens if 
column == -1?



src/kcolumnresizer.cpp


You can use auto here, if wanted (you can use auto much more often in your 
patch, btw.):

if (auto gridLayout = qobject_cast(layout)) {
d->addWidgetsFromGridLayout(gridLayout, column);
return;
}

if (auto formLayout = qobject_cast(layout)) {
// ...
}


- Dominik Haumann


On June 4, 2014, 9:52 a.m., Aurélien Gâteau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118514/
> ---
> 
> (Updated June 4, 2014, 9:52 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> I have been asked to add ColumnResizer to KWidgetAddons. ColumnResizer is a 
> helper class I created long ago to help maintain uniform widths across 
> layouts. You can learn more about it from the blog post I wrote back then: 
> http://agateau.com/2011/clean-up-your-layouts-with-columnresizer/ . I 
> simplified the API a bit, added some documentation, and here it is.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 27b9084 
>   src/kcolumnresizer.h PRE-CREATION 
>   src/kcolumnresizer.cpp PRE-CREATION 
>   tests/CMakeLists.txt eccf887 
>   tests/kcolumnresizertest.h PRE-CREATION 
>   tests/kcolumnresizertest.cpp PRE-CREATION 
>   tests/kcolumnresizertest.ui PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/118514/diff/
> 
> 
> Testing
> ---
> 
> Manual test program behaves as expected.
> 
> 
> Thanks,
> 
> Aurélien Gâteau
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118514: Add KColumnResizer to KWidgetsAddons

[Christoph Feck]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118514/#review59163
---


Regarding "remove" type functions, I agree we need them.

Please also check if adding a widget multiple times does work.

There could also be an "addLayout()" (and removeLayout()) function, and on each 
"update", it would check which widgets are in there, and handle them.


src/kcolumnresizer.h


Does it make sense to make "= 0" the default value for the column?



src/kcolumnresizer.cpp


I think our style guide requires indenting the initializers.



src/kcolumnresizer.cpp


indent



src/kcolumnresizer.cpp


indent



src/kcolumnresizer.cpp


What would be nice is if the widget you add is actually a GroupBox 
containing a supported layout, then it would internally call 
addWidgetsFromLayout().

Remains the issue with layouts whose children could change...


- Christoph Feck


On June 4, 2014, 9:52 a.m., Aurélien Gâteau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118514/
> ---
> 
> (Updated June 4, 2014, 9:52 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> I have been asked to add ColumnResizer to KWidgetAddons. ColumnResizer is a 
> helper class I created long ago to help maintain uniform widths across 
> layouts. You can learn more about it from the blog post I wrote back then: 
> http://agateau.com/2011/clean-up-your-layouts-with-columnresizer/ . I 
> simplified the API a bit, added some documentation, and here it is.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 27b9084 
>   src/kcolumnresizer.h PRE-CREATION 
>   src/kcolumnresizer.cpp PRE-CREATION 
>   tests/CMakeLists.txt eccf887 
>   tests/kcolumnresizertest.h PRE-CREATION 
>   tests/kcolumnresizertest.cpp PRE-CREATION 
>   tests/kcolumnresizertest.ui PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/118514/diff/
> 
> 
> Testing
> ---
> 
> Manual test program behaves as expected.
> 
> 
> Thanks,
> 
> Aurélien Gâteau
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118514: Add KColumnResizer to KWidgetsAddons

[Christoph Feck]

> On June 4, 2014, 11:41 a.m., Dominik Haumann wrote:
> > What about adding something like this to Qt ?

First, I was thinking the same. The efforts with KF5 work was to get as much 
features as possible into Qt, and only keep the stuff that isn't accepted in 
our libraries.

But here, I am not really sure. Is the alignment requirement specific to KDE's 
guidelines?


- Christoph


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118514/#review59170
---


On June 4, 2014, 9:52 a.m., Aurélien Gâteau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118514/
> ---
> 
> (Updated June 4, 2014, 9:52 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> I have been asked to add ColumnResizer to KWidgetAddons. ColumnResizer is a 
> helper class I created long ago to help maintain uniform widths across 
> layouts. You can learn more about it from the blog post I wrote back then: 
> http://agateau.com/2011/clean-up-your-layouts-with-columnresizer/ . I 
> simplified the API a bit, added some documentation, and here it is.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 27b9084 
>   src/kcolumnresizer.h PRE-CREATION 
>   src/kcolumnresizer.cpp PRE-CREATION 
>   tests/CMakeLists.txt eccf887 
>   tests/kcolumnresizertest.h PRE-CREATION 
>   tests/kcolumnresizertest.cpp PRE-CREATION 
>   tests/kcolumnresizertest.ui PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/118514/diff/
> 
> 
> Testing
> ---
> 
> Manual test program behaves as expected.
> 
> 
> Thanks,
> 
> Aurélien Gâteau
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118514: Add KColumnResizer to KWidgetsAddons

[Aurélien Gâteau]

> On June 4, 2014, 1:52 p.m., Christoph Feck wrote:
> > src/kcolumnresizer.cpp, line 182
> > 
> >
> > What would be nice is if the widget you add is actually a GroupBox 
> > containing a supported layout, then it would internally call 
> > addWidgetsFromLayout().
> > 
> > Remains the issue with layouts whose children could change...

I disagree with this as it would make it impossible to add the groupbox itself 
so that its width is controlled. If you want to align the content of a group 
box, just do so with addWidgetsFromLayout(groupBox->layout()).


- Aurélien


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118514/#review59163
---


On June 4, 2014, 11:52 a.m., Aurélien Gâteau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118514/
> ---
> 
> (Updated June 4, 2014, 11:52 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> I have been asked to add ColumnResizer to KWidgetAddons. ColumnResizer is a 
> helper class I created long ago to help maintain uniform widths across 
> layouts. You can learn more about it from the blog post I wrote back then: 
> http://agateau.com/2011/clean-up-your-layouts-with-columnresizer/ . I 
> simplified the API a bit, added some documentation, and here it is.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 27b9084 
>   src/kcolumnresizer.h PRE-CREATION 
>   src/kcolumnresizer.cpp PRE-CREATION 
>   tests/CMakeLists.txt eccf887 
>   tests/kcolumnresizertest.h PRE-CREATION 
>   tests/kcolumnresizertest.cpp PRE-CREATION 
>   tests/kcolumnresizertest.ui PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/118514/diff/
> 
> 
> Testing
> ---
> 
> Manual test program behaves as expected.
> 
> 
> Thanks,
> 
> Aurélien Gâteau
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118514: Add KColumnResizer to KWidgetsAddons

[Dominik Haumann]

> On June 4, 2014, 11:41 a.m., Dominik Haumann wrote:
> > What about adding something like this to Qt ?
> 
> Christoph Feck wrote:
> First, I was thinking the same. The efforts with KF5 work was to get as 
> much features as possible into Qt, and only keep the stuff that isn't 
> accepted in our libraries.
> 
> But here, I am not really sure. Is the alignment requirement specific to 
> KDE's guidelines?

Reading the comments in 
http://agateau.com/2011/clean-up-your-layouts-with-columnresizer/ there seems 
to be quite a lot of interest in this, some also saying it should be in Qt.


- Dominik


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118514/#review59170
---


On June 4, 2014, 9:52 a.m., Aurélien Gâteau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118514/
> ---
> 
> (Updated June 4, 2014, 9:52 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> I have been asked to add ColumnResizer to KWidgetAddons. ColumnResizer is a 
> helper class I created long ago to help maintain uniform widths across 
> layouts. You can learn more about it from the blog post I wrote back then: 
> http://agateau.com/2011/clean-up-your-layouts-with-columnresizer/ . I 
> simplified the API a bit, added some documentation, and here it is.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 27b9084 
>   src/kcolumnresizer.h PRE-CREATION 
>   src/kcolumnresizer.cpp PRE-CREATION 
>   tests/CMakeLists.txt eccf887 
>   tests/kcolumnresizertest.h PRE-CREATION 
>   tests/kcolumnresizertest.cpp PRE-CREATION 
>   tests/kcolumnresizertest.ui PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/118514/diff/
> 
> 
> Testing
> ---
> 
> Manual test program behaves as expected.
> 
> 
> Thanks,
> 
> Aurélien Gâteau
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118514: Add KColumnResizer to KWidgetsAddons

[Aurélien Gâteau]

> On June 4, 2014, 12:01 p.m., David Edmundson wrote:
> > src/kcolumnresizer.cpp, line 214
> > 
> >
> > Do we really want it to be critical?
> > 
> > Sometimes widgets are loaded dynamically, we probably don't want to 
> > explode because some widgets might not line up.
> > 
> > Maybe a qWarning + a Q_ASSERT.
> >

Fixed. Note that qCritical() does not abort though, qFatal() does.


- Aurélien


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118514/#review59145
---


On June 4, 2014, 11:52 a.m., Aurélien Gâteau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118514/
> ---
> 
> (Updated June 4, 2014, 11:52 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> I have been asked to add ColumnResizer to KWidgetAddons. ColumnResizer is a 
> helper class I created long ago to help maintain uniform widths across 
> layouts. You can learn more about it from the blog post I wrote back then: 
> http://agateau.com/2011/clean-up-your-layouts-with-columnresizer/ . I 
> simplified the API a bit, added some documentation, and here it is.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 27b9084 
>   src/kcolumnresizer.h PRE-CREATION 
>   src/kcolumnresizer.cpp PRE-CREATION 
>   tests/CMakeLists.txt eccf887 
>   tests/kcolumnresizertest.h PRE-CREATION 
>   tests/kcolumnresizertest.cpp PRE-CREATION 
>   tests/kcolumnresizertest.ui PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/118514/diff/
> 
> 
> Testing
> ---
> 
> Manual test program behaves as expected.
> 
> 
> Thanks,
> 
> Aurélien Gâteau
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118514: Add KColumnResizer to KWidgetsAddons

[Aurélien Gâteau]

> On June 4, 2014, 1:41 p.m., Dominik Haumann wrote:
> > src/kcolumnresizer.cpp, line 200
> > 
> >
> > You can use auto here, if wanted (you can use auto much more often in 
> > your patch, btw.):
> > 
> > if (auto gridLayout = qobject_cast(layout)) {
> > d->addWidgetsFromGridLayout(gridLayout, column);
> > return;
> > }
> > 
> > if (auto formLayout = qobject_cast(layout)) {
> > // ...
> > }

I am not fond of declaring variables inside the condition part of an if: I find 
those harder to read.


- Aurélien


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118514/#review59170
---


On June 4, 2014, 11:52 a.m., Aurélien Gâteau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118514/
> ---
> 
> (Updated June 4, 2014, 11:52 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> I have been asked to add ColumnResizer to KWidgetAddons. ColumnResizer is a 
> helper class I created long ago to help maintain uniform widths across 
> layouts. You can learn more about it from the blog post I wrote back then: 
> http://agateau.com/2011/clean-up-your-layouts-with-columnresizer/ . I 
> simplified the API a bit, added some documentation, and here it is.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 27b9084 
>   src/kcolumnresizer.h PRE-CREATION 
>   src/kcolumnresizer.cpp PRE-CREATION 
>   tests/CMakeLists.txt eccf887 
>   tests/kcolumnresizertest.h PRE-CREATION 
>   tests/kcolumnresizertest.cpp PRE-CREATION 
>   tests/kcolumnresizertest.ui PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/118514/diff/
> 
> 
> Testing
> ---
> 
> Manual test program behaves as expected.
> 
> 
> Thanks,
> 
> Aurélien Gâteau
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118514: Add KColumnResizer to KWidgetsAddons

[Aurélien Gâteau]

> On June 4, 2014, 1:52 p.m., Christoph Feck wrote:
> > src/kcolumnresizer.h, line 93
> > 
> >
> > Does it make sense to make "= 0" the default value for the column?

Sounds good to me. Just did so.


- Aurélien


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118514/#review59163
---


On June 4, 2014, 11:52 a.m., Aurélien Gâteau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118514/
> ---
> 
> (Updated June 4, 2014, 11:52 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> I have been asked to add ColumnResizer to KWidgetAddons. ColumnResizer is a 
> helper class I created long ago to help maintain uniform widths across 
> layouts. You can learn more about it from the blog post I wrote back then: 
> http://agateau.com/2011/clean-up-your-layouts-with-columnresizer/ . I 
> simplified the API a bit, added some documentation, and here it is.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 27b9084 
>   src/kcolumnresizer.h PRE-CREATION 
>   src/kcolumnresizer.cpp PRE-CREATION 
>   tests/CMakeLists.txt eccf887 
>   tests/kcolumnresizertest.h PRE-CREATION 
>   tests/kcolumnresizertest.cpp PRE-CREATION 
>   tests/kcolumnresizertest.ui PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/118514/diff/
> 
> 
> Testing
> ---
> 
> Manual test program behaves as expected.
> 
> 
> Thanks,
> 
> Aurélien Gâteau
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118514: Add KColumnResizer to KWidgetsAddons

[Aurélien Gâteau]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118514/
---

(Updated June 4, 2014, 2:43 p.m.)


Review request for KDE Frameworks and Christoph Feck.


Changes
---

Fixed many reported issues.


Repository: kwidgetsaddons


Description
---

I have been asked to add ColumnResizer to KWidgetAddons. ColumnResizer is a 
helper class I created long ago to help maintain uniform widths across layouts. 
You can learn more about it from the blog post I wrote back then: 
http://agateau.com/2011/clean-up-your-layouts-with-columnresizer/ . I 
simplified the API a bit, added some documentation, and here it is.


Diffs (updated)
-

  src/kcolumnresizer.cpp PRE-CREATION 
  src/CMakeLists.txt 27b9084 
  src/kcolumnresizer.h PRE-CREATION 
  tests/CMakeLists.txt eccf887 
  tests/kcolumnresizertest.h PRE-CREATION 
  tests/kcolumnresizertest.cpp PRE-CREATION 
  tests/kcolumnresizertest.ui PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/118514/diff/


Testing
---

Manual test program behaves as expected.


Thanks,

Aurélien Gâteau

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 118526: Provide i18nd wrappers in kdeclarative

[Martin Gräßlin]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118526/
---

Review request for KDE Frameworks, Plasma and Marco Martin.


Repository: kdeclarative


Description
---

Provide i18nd wrappers in kdeclarative

As QML might combine multiple modules with different cataloges we need
to be able to specify the translation domain explicitly. If there is a
need to use a specific domain for all i18n calls (e.g. in a library
using QML) there is the possibility to set a global translation domain
through KDeclarative. If such a domain is set all i18n calls delegate
to the i18nd variant.

Due to the nature of KDeclarative we cannot mix i18n calls with
different domains. If two modules would require to set the translation
domain it's bound to fail. Thus the recommendation is to use the i18nd
variants in any QML code which is intended to be used as an import.


Diffs
-

  src/kdeclarative/kdeclarative.h b4a274b710f4de7ffbfc275d1e9a0a93be283053 
  src/kdeclarative/kdeclarative.cpp a35dac5cfbd42e75e892d4ad88c491345be4a1b0 
  src/kdeclarative/private/kdeclarative_p.h 
6b61d123bf74671b413e4e68bf911bb969fdaf53 
  src/kdeclarative/private/rootcontext.cpp 
12309b096495910b83ed1e388989042b45a1 
  src/kdeclarative/private/rootcontext_p.h 
16694b155c668e11cf7a16549a18cdc89b81b3e2 

Diff: https://git.reviewboard.kde.org/r/118526/diff/


Testing
---

adjusted kwineffects KCM and run it with the x-test language: the strings with 
i18n from QML side are now picking up the translated strings.


Thanks,

Martin Gräßlin

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Jenkins build is back to stable : ktexteditor_master_qt5 #417

[KDE CI System]

See 

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118458: Fix KDirWatch to act more reliably against various backends.

[Matthew Dawson]

> On June 4, 2014, 3:38 a.m., David Faure wrote:
> > autotests/kdirwatch_unittest.cpp, line 523
> > 
> >
> > The commit log doesn't mention any changes related to 
> > QFileSystemWatcher, only to the FAM backend. Are you sure you wanted to 
> > commit this? If it works then of course I'm happy with it (but maybe in a 
> > separate commit then)

I'm not sure how that got fixed, I just seem to remember changing things and in 
the process of working on that test I discovered that QFSW worked just fine.  I 
don't remember if my changes did that, or if it was fixed sometime ago and I 
just discovered the fact.  Or if Qt5 fixed it?

I'll move it to a separate commit to be safe.


> On June 4, 2014, 3:38 a.m., David Faure wrote:
> > autotests/kdirwatch_unittest.cpp, line 677
> > 
> >
> > This could actually use QFile::link(), would be more readable.

So, here be dragons.  QFile::link fails the unit test.  But ::link doesn't.  
I'm going to leave this alone for now.


> On June 4, 2014, 3:38 a.m., David Faure wrote:
> > src/lib/io/kdirwatch.cpp, line 636
> > 
> >
> > I'm not 100% sure about the meaning of this boolean variable. (we 
> > started an event?)
> > Maybe it can be called something more explicit 
> > (waitForInitialFAMEvents? or something that makes sense to you ;))

I'm not sure what this should be.  I think startedFAMMonitor is more clear, as 
it says what the variable is tracking, as opposed to what happens when set.  It 
marks when a FAMMonitor* function is called, which matches.  If you prefer the 
waitForInitialFAMEvents, signalling what will happen, I'll it to that.

You did basically guess what the variable meant, but it really speaks more to 
the FAM Monitor piece, not an event.


> On June 4, 2014, 3:38 a.m., David Faure wrote:
> > src/lib/io/kdirwatch.cpp, line 1633
> > 
> >
> > Technically this could be an event about a completely unrelated file to 
> > the one we're waiting to see, couldn't it?
> >

As far as I understand the code, no.  FAM reports which request the FAM event 
is associated with, which each KDirWatch event stores.  So this FAMEndExists 
(or similar) will be associated with a single KDW event, and thus only update 
the correct one.


> On June 4, 2014, 3:38 a.m., David Faure wrote:
> > src/lib/io/kdirwatch.cpp, line 1719
> > 
> >
> > actual -> actually ?

Whoops, fixed.


> On June 4, 2014, 3:38 a.m., David Faure wrote:
> > autotests/kdirwatch_unittest.cpp, line 297
> > 
> >
> > It seems that the second call should pass "created(QString)" rather 
> > than "deleted(QString)".

Copy and paste errors are the worst ...


- Matthew


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118458/#review59123
---


On June 1, 2014, 5:03 p.m., Matthew Dawson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118458/
> ---
> 
> (Updated June 1, 2014, 5:03 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Bugs: 329802
> https://bugs.kde.org/show_bug.cgi?id=329802
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Fix KDirWatch to act more reliably against various backends.
> 
>  - When KDirWatch used a FAM backend, it wouldn't actually wait for the
> necessary watches to be put in place before returning, allowing for a race
> where an application may think a watch is in place when it really isn't.
> This was easily seen using gamin and running the testDeleteAndRecreateFile
> test.  Fix by delaying useFAM until the watch is in place by waiting until
> the EndExist FAM event is received.  This adds ~100-200ms per watch with
> gamin.
>  - When a file is deleted and recreated, and scanEntry detects it, the dirty
> signal would be emitted.  Fix to emit a deleted + created signal, as expected.
>  - When a deleted signal was requested, other signals were dropped.  Due to
> the above point, this would stop the created signal from being emitted.  Now,
> all signals are emitted, even when a delete is detected.
>  - When watching recreated files, the created signal could get lost as there
> was a race between when the created signal would be generated and its signal
> spy would be created.  Fix by making sure the spy is created before the signa

Re: Review Request 118458: Fix KDirWatch to act more reliably against various backends.

[Matthew Dawson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118458/
---

(Updated June 4, 2014, 1:05 p.m.)


Review request for KDE Frameworks.


Changes
---

Fix patch to resolve issues raised.


Bugs: 329802
https://bugs.kde.org/show_bug.cgi?id=329802


Repository: kcoreaddons


Description
---

Fix KDirWatch to act more reliably against various backends.

 - When KDirWatch used a FAM backend, it wouldn't actually wait for the
necessary watches to be put in place before returning, allowing for a race
where an application may think a watch is in place when it really isn't.
This was easily seen using gamin and running the testDeleteAndRecreateFile
test.  Fix by delaying useFAM until the watch is in place by waiting until
the EndExist FAM event is received.  This adds ~100-200ms per watch with
gamin.
 - When a file is deleted and recreated, and scanEntry detects it, the dirty
signal would be emitted.  Fix to emit a deleted + created signal, as expected.
 - When a deleted signal was requested, other signals were dropped.  Due to
the above point, this would stop the created signal from being emitted.  Now,
all signals are emitted, even when a delete is detected.
 - When watching recreated files, the created signal could get lost as there
was a race between when the created signal would be generated and its signal
spy would be created.  Fix by making sure the spy is created before the signal
could be emitted.
 - Make sure the created signal isn't emitted twice with both FAM and the
polling timer.  This occurs as FAM doesn't fix up the fact the entry has been
created, and the poller thus assumes it needs to notify the world.  Fix by
having FAM not emit the event in this case.

BUG: 329802

Note that due to a bug in the FAM daemon (which doesn't exist in the gamin 
daemon), the testDeleteAndRecreateDir unit test fails as FAM doesn't detect the 
folder deletion.  This test previously passed due to the race condition 
outlined in bug 329802, which was fixed in this RR.  I've confirmed this is a 
bug in FAM using a minimal program talking directly to the daemon, and watching 
the strace output from FAM.  FAM doesn't detect when a folder it is watching 
directly is deleted.  It does detect when a folder inside a folder it is 
watching is deleted, however.
I think this should go in as is, as the unit test failing exposes the actual 
problem, instead of hiding behind a race when it doesn't really work.  If 
running against FAM is important, I can see about tracking down the issue.


Diffs (updated)
-

  autotests/kdirwatch_unittest.cpp 4e638e089012276dc8b17b9d960732a161cc3be1 
  src/lib/io/kdirwatch.cpp cc6feec4d2b2598e50c853172bbc295a8c719bc2 
  src/lib/io/kdirwatch_p.h cf63c41745091cfedcf1b27c9e4aa77d3db6b31a 

Diff: https://git.reviewboard.kde.org/r/118458/diff/


Testing
---

Unit tests all pass now with gamin.  Using FAM, one test fails as described 
above, but I've confirmed the daemon itself is causing the issue.


Thanks,

Matthew Dawson

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Jenkins build is back to stable : kdelibs_stable #1102

[KDE CI System]

See 

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 118538: Add a property containing the real edge a dialog is shown on

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118538/
---

Review request for KDE Frameworks and Plasma.


Repository: plasma-framework


Description
---

Add a property containing the real edge a dialog is shown on.

If a dialog is meant to be shown on the left of an item, if there is not
enough space it will be shown on the right.

We need to keep track of where we're actually shown in order to show the
correct borders and perform the animation in the correct direction.

It is exposed as a property in case the dialog itself needs to know
which side of the parent item it is, for example if needs to show an
arrow to the parent.

--

Fixed some serious copy + paste fails in the hitting the bottom edge of
the screen check.


Diffs
-

  src/plasmaquick/dialog.h 4009222 
  src/plasmaquick/dialog.cpp 2f8227c 

Diff: https://git.reviewboard.kde.org/r/118538/diff/


Testing
---

Ran dialog_positioning.qml in all 8 combinations of edges + preferred locations.
(i.e placed near left edge, showed item with location set to both left and 
right)


Thanks,

David Edmundson

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118538: Add a property containing the real edge a dialog is shown on

[Marco Martin]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118538/#review59234
---


what is a valid use case where qml needs to know where the dialog actually 
is?(not hypothetical please)
doing the animation on the proper direction can be tracked completely internally

also, plasmoids should never ever do a screen edge animation when in the middle 
of the desktop, the animation looks bad where it's clipped if it's not against 
a panel or a screen edge

- Marco Martin


On June 4, 2014, 7:07 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118538/
> ---
> 
> (Updated June 4, 2014, 7:07 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Add a property containing the real edge a dialog is shown on.
> 
> If a dialog is meant to be shown on the left of an item, if there is not
> enough space it will be shown on the right.
> 
> We need to keep track of where we're actually shown in order to show the
> correct borders and perform the animation in the correct direction.
> 
> It is exposed as a property in case the dialog itself needs to know
> which side of the parent item it is, for example if needs to show an
> arrow to the parent.
> 
> --
> 
> Fixed some serious copy + paste fails in the hitting the bottom edge of
> the screen check.
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/dialog.h 4009222 
>   src/plasmaquick/dialog.cpp 2f8227c 
> 
> Diff: https://git.reviewboard.kde.org/r/118538/diff/
> 
> 
> Testing
> ---
> 
> Ran dialog_positioning.qml in all 8 combinations of edges + preferred 
> locations.
> (i.e placed near left edge, showed item with location set to both left and 
> right)
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118538: Add a property containing the real edge a dialog is shown on

[David Edmundson]

> On June 4, 2014, 7:30 p.m., Marco Martin wrote:
> > what is a valid use case where qml needs to know where the dialog actually 
> > is?(not hypothetical please)
> > doing the animation on the proper direction can be tracked completely 
> > internally
> > 
> > also, plasmoids should never ever do a screen edge animation when in the 
> > middle of the desktop, the animation looks bad where it's clipped if it's 
> > not against a panel or a screen edge

Milou places the most relevant item closest to the launcher.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118538/#review59234
---


On June 4, 2014, 7:07 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118538/
> ---
> 
> (Updated June 4, 2014, 7:07 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Add a property containing the real edge a dialog is shown on.
> 
> If a dialog is meant to be shown on the left of an item, if there is not
> enough space it will be shown on the right.
> 
> We need to keep track of where we're actually shown in order to show the
> correct borders and perform the animation in the correct direction.
> 
> It is exposed as a property in case the dialog itself needs to know
> which side of the parent item it is, for example if needs to show an
> arrow to the parent.
> 
> --
> 
> Fixed some serious copy + paste fails in the hitting the bottom edge of
> the screen check.
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/dialog.h 4009222 
>   src/plasmaquick/dialog.cpp 2f8227c 
> 
> Diff: https://git.reviewboard.kde.org/r/118538/diff/
> 
> 
> Testing
> ---
> 
> Ran dialog_positioning.qml in all 8 combinations of edges + preferred 
> locations.
> (i.e placed near left edge, showed item with location set to both left and 
> right)
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118270: [doc] explicitly load external entities (after CVE-2014-0191)

[Commit Hook]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118270/#review59243
---


This review has been submitted with commit 
d4fca9ffb31a2383459c89b27f81b10b7ddece1a by Luigi Toscano to branch KDE/4.13.

- Commit Hook


On June 3, 2014, 1:50 p.m., Luigi Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118270/
> ---
> 
> (Updated June 3, 2014, 1:50 p.m.)
> 
> 
> Review request for Documentation, KDE Frameworks, kdelibs, Rohan Garg, 
> Jonathan Riddell, Luc Menut, and Rex Dieter.
> 
> 
> Bugs: 335001
> http://bugs.kde.org/show_bug.cgi?id=335001
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Use the more modern API function for XML loading and enable the flags which 
> load the external entities, so that meinproc4 can work
> again after the security changes implemented for CVE-2014-0191.
> Without this change meinproc4 complains (see the referenced bug)
> 
> The fix (half of the patch, the other half is on code which was removed) 
> applies to KF5 too, hence the group.
> 
> My tests shows that the documentation cache is properly generated as before, 
> and the patch should work even on the old 
> 
> Packagers (Ubuntu packagers in CC, as Ubuntu is one of the few distributions 
> where libxml2 has been already patched) could you please test it with a fixed 
> libxml and without, and if possible with KF5 as well?
> 
> 
> Diffs
> -
> 
>   kdoctools/meinproc.cpp 0894d63 
>   kdoctools/xslt.cpp a7265ca 
> 
> Diff: https://git.reviewboard.kde.org/r/118270/diff/
> 
> 
> Testing
> ---
> 
> meinproc4 works again
> 
> 
> Thanks,
> 
> Luigi Toscano
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118270: [doc] explicitly load external entities (after CVE-2014-0191)

[Luigi Toscano]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118270/
---

(Updated June 4, 2014, 8:40 p.m.)


Status
--

This change has been marked as submitted.


Review request for Documentation, KDE Frameworks, kdelibs, Rohan Garg, Jonathan 
Riddell, Luc Menut, and Rex Dieter.


Bugs: 335001
http://bugs.kde.org/show_bug.cgi?id=335001


Repository: kdelibs


Description
---

Use the more modern API function for XML loading and enable the flags which 
load the external entities, so that meinproc4 can work
again after the security changes implemented for CVE-2014-0191.
Without this change meinproc4 complains (see the referenced bug)

The fix (half of the patch, the other half is on code which was removed) 
applies to KF5 too, hence the group.

My tests shows that the documentation cache is properly generated as before, 
and the patch should work even on the old 

Packagers (Ubuntu packagers in CC, as Ubuntu is one of the few distributions 
where libxml2 has been already patched) could you please test it with a fixed 
libxml and without, and if possible with KF5 as well?


Diffs
-

  kdoctools/meinproc.cpp 0894d63 
  kdoctools/xslt.cpp a7265ca 

Diff: https://git.reviewboard.kde.org/r/118270/diff/


Testing
---

meinproc4 works again


Thanks,

Luigi Toscano

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118538: Add a property containing the real edge a dialog is shown on

[Marco Martin]

> On June 4, 2014, 7:30 p.m., Marco Martin wrote:
> > what is a valid use case where qml needs to know where the dialog actually 
> > is?(not hypothetical please)
> > doing the animation on the proper direction can be tracked completely 
> > internally
> > 
> > also, plasmoids should never ever do a screen edge animation when in the 
> > middle of the desktop, the animation looks bad where it's clipped if it's 
> > not against a panel or a screen edge
> 
> David Edmundson wrote:
> Milou places the most relevant item closest to the launcher.
> 
>

Hmm, it would be relevant only in the case milou is iconified and in the 
desktop, that seems quite a narrow case.
api wise it may make sense a Plasma::Types::Direction visualParentDirection, 
*but* it would have to be exported in plasmoid as well, and I don't want 
anything like that here.
API that is added ad hoc for a single, extremely narrow use case is never a 
good idea.


- Marco


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118538/#review59234
---


On June 4, 2014, 7:07 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118538/
> ---
> 
> (Updated June 4, 2014, 7:07 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Add a property containing the real edge a dialog is shown on.
> 
> If a dialog is meant to be shown on the left of an item, if there is not
> enough space it will be shown on the right.
> 
> We need to keep track of where we're actually shown in order to show the
> correct borders and perform the animation in the correct direction.
> 
> It is exposed as a property in case the dialog itself needs to know
> which side of the parent item it is, for example if needs to show an
> arrow to the parent.
> 
> --
> 
> Fixed some serious copy + paste fails in the hitting the bottom edge of
> the screen check.
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/dialog.h 4009222 
>   src/plasmaquick/dialog.cpp 2f8227c 
> 
> Diff: https://git.reviewboard.kde.org/r/118538/diff/
> 
> 
> Testing
> ---
> 
> Ran dialog_positioning.qml in all 8 combinations of edges + preferred 
> locations.
> (i.e placed near left edge, showed item with location set to both left and 
> right)
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 118546: No symlink to common/ directory for documentation common files (broken, now unused, UNIX only)

[Luigi Toscano]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118546/
---

Review request for Documentation and KDE Frameworks.


Repository: kdoctools


Description
---

The common/ directory (which contains per-language common files for 
documentation) has been renamed (kdoctools5-common), and even if it was not, 
the symlink is not needed anymore to retrieve  the documentation (the 
references to the content in that directory now are accessed through help:/ 
kioslave).
The removal of the need for this symlink solve a long-standing TODO for 
non-UNIX platform(s). 


Diffs
-

  KF5DocToolsMacros.cmake 823da90 

Diff: https://git.reviewboard.kde.org/r/118546/diff/


Testing
---

Documentation generated, help:/ kioslave still working


Thanks,

Luigi Toscano

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118538: Add a property containing the real edge a dialog is shown on

[David Edmundson]

> On June 4, 2014, 7:30 p.m., Marco Martin wrote:
> > what is a valid use case where qml needs to know where the dialog actually 
> > is?(not hypothetical please)
> > doing the animation on the proper direction can be tracked completely 
> > internally
> > 
> > also, plasmoids should never ever do a screen edge animation when in the 
> > middle of the desktop, the animation looks bad where it's clipped if it's 
> > not against a panel or a screen edge
> 
> David Edmundson wrote:
> Milou places the most relevant item closest to the launcher.
> 
>
> 
> Marco Martin wrote:
> Hmm, it would be relevant only in the case milou is iconified and in the 
> desktop, that seems quite a narrow case.
> api wise it may make sense a Plasma::Types::Direction 
> visualParentDirection, *but* it would have to be exported in plasmoid as 
> well, and I don't want anything like that here.
> API that is added ad hoc for a single, extremely narrow use case is never 
> a good idea.

kickoff changes direction too so the toolbar is closest to the launcher.

However, I'm not too fussed about the new property, I can remove the Q_PROPERTY 
and add it if we decide it's needed. The important part for me is making it 
show the correct borders and animate in the right direction.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118538/#review59234
---


On June 4, 2014, 7:07 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118538/
> ---
> 
> (Updated June 4, 2014, 7:07 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Add a property containing the real edge a dialog is shown on.
> 
> If a dialog is meant to be shown on the left of an item, if there is not
> enough space it will be shown on the right.
> 
> We need to keep track of where we're actually shown in order to show the
> correct borders and perform the animation in the correct direction.
> 
> It is exposed as a property in case the dialog itself needs to know
> which side of the parent item it is, for example if needs to show an
> arrow to the parent.
> 
> --
> 
> Fixed some serious copy + paste fails in the hitting the bottom edge of
> the screen check.
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/dialog.h 4009222 
>   src/plasmaquick/dialog.cpp 2f8227c 
> 
> Diff: https://git.reviewboard.kde.org/r/118538/diff/
> 
> 
> Testing
> ---
> 
> Ran dialog_positioning.qml in all 8 combinations of edges + preferred 
> locations.
> (i.e placed near left edge, showed item with location set to both left and 
> right)
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118538: Add a property containing the real edge a dialog is shown on

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118538/
---

(Updated June 4, 2014, 10:43 p.m.)


Review request for KDE Frameworks and Plasma.


Repository: plasma-framework


Description
---

Add a property containing the real edge a dialog is shown on.

If a dialog is meant to be shown on the left of an item, if there is not
enough space it will be shown on the right.

We need to keep track of where we're actually shown in order to show the
correct borders and perform the animation in the correct direction.

It is exposed as a property in case the dialog itself needs to know
which side of the parent item it is, for example if needs to show an
arrow to the parent.

--

Fixed some serious copy + paste fails in the hitting the bottom edge of
the screen check.


Diffs (updated)
-

  src/plasmaquick/dialog.cpp 2f8227c 
  src/plasmaquick/dialog.h 4009222 

Diff: https://git.reviewboard.kde.org/r/118538/diff/


Testing
---

Ran dialog_positioning.qml in all 8 combinations of edges + preferred locations.
(i.e placed near left edge, showed item with location set to both left and 
right)


Thanks,

David Edmundson

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 118547: Expose Formats as singleton

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118547/
---

Review request for KDE Frameworks and Plasma.


Repository: kdeclarative


Description
---

Expose Formats as singleton

Formats is basically just a collection of invokable static methods.
This saves creating a few objects and makes for a a nicer API.

Without it you have to have a bit of code like:

-KCoreAddons.Formats {
-id: formats
-}
then do text:formats.formatData(...) in your labels

this becomes now

KCoreAddons.Format.formatData(...) 


Diffs
-

  src/qmlcontrols/kcoreaddons/kcoreaddonsplugin.cpp 65dd75f 

Diff: https://git.reviewboard.kde.org/r/118547/diff/


Testing
---

Made the two changes needed in plasma-workspace. Everything seemed to be ok.


Thanks,

David Edmundson

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118547: Expose Formats as singleton

[Mark Gaiser]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118547/#review59256
---

Ship it!


Makes sense and looks nice!

- Mark Gaiser


On June 5, 2014, 12:03 a.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118547/
> ---
> 
> (Updated June 5, 2014, 12:03 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kdeclarative
> 
> 
> Description
> ---
> 
> Expose Formats as singleton
> 
> Formats is basically just a collection of invokable static methods.
> This saves creating a few objects and makes for a a nicer API.
> 
> Without it you have to have a bit of code like:
> 
> -KCoreAddons.Formats {
> -id: formats
> -}
> then do text:formats.formatData(...) in your labels
> 
> this becomes now
> 
> KCoreAddons.Format.formatData(...) 
> 
> 
> Diffs
> -
> 
>   src/qmlcontrols/kcoreaddons/kcoreaddonsplugin.cpp 65dd75f 
> 
> Diff: https://git.reviewboard.kde.org/r/118547/diff/
> 
> 
> Testing
> ---
> 
> Made the two changes needed in plasma-workspace. Everything seemed to be ok.
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118547: Expose Formats as singleton

[Bhushan Shah]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118547/#review59261
---



src/qmlcontrols/kcoreaddons/kcoreaddonsplugin.cpp


Err no, this will fail when using formatRelativeDate and 
formatRelativeDateTime


- Bhushan Shah


On June 5, 2014, 5:33 a.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118547/
> ---
> 
> (Updated June 5, 2014, 5:33 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kdeclarative
> 
> 
> Description
> ---
> 
> Expose Formats as singleton
> 
> Formats is basically just a collection of invokable static methods.
> This saves creating a few objects and makes for a a nicer API.
> 
> Without it you have to have a bit of code like:
> 
> -KCoreAddons.Formats {
> -id: formats
> -}
> then do text:formats.formatData(...) in your labels
> 
> this becomes now
> 
> KCoreAddons.Format.formatData(...) 
> 
> 
> Diffs
> -
> 
>   src/qmlcontrols/kcoreaddons/kcoreaddonsplugin.cpp 65dd75f 
> 
> Diff: https://git.reviewboard.kde.org/r/118547/diff/
> 
> 
> Testing
> ---
> 
> Made the two changes needed in plasma-workspace. Everything seemed to be ok.
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118490: Properly mark ServiceBrowser::isRunning as deprecated.

[Matthew Dawson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118490/
---

(Updated June 4, 2014, 11:56 p.m.)


Review request for KDE Frameworks and Matthew Dawson.


Changes
---

New version, taking into account all comments.  I'd appreciate it if someone 
can have a quick look over the comment to verify it makes sense.


Summary (updated)
-

Properly mark ServiceBrowser::isRunning as deprecated.


Repository: kdnssd


Description (updated)
---

Properly mark ServiceBrowser::isRunning as deprecated.

Fix the @deprecated notation to be correct.  Also, make the function a simple
inline function that always returns false.  This should cause minimal breakage
while avoiding overhead from the function.


Diffs (updated)
-

  src/mdnsd-servicetypebrowser.cpp d16dd1fa1c692cd20e21f1a7d5471ac7956469b3 
  src/dummy-servicetypebrowser.cpp 39a4b6b660bb47b68b5721535994487dbe2abbf6 
  src/avahi-servicetypebrowser.cpp b3c14f84367d5093a6cee5ef3f684edc201a3f96 
  src/servicetypebrowser.h 3cd10e36bf089e088e8a48bb0cf73daa4fb859c5 

Diff: https://git.reviewboard.kde.org/r/118490/diff/


Testing
---

Code still compiles.


Thanks,

Matthew Dawson

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel