D29347: KAuthorized: export method to reload restrictions

[Matthew Dawson]

mdawson added a comment.


  One suggestion for this change:
  Instead of exporting a method that takes no parameters and always loads from 
configuration file, why not make a new method with the implementation that 
takes in a given KConfigGroup.  That way unit tests can pass in a KConfigGroup 
setup appropriately without having to create a normal configuration file in the 
user's home folder (ie use a temporary file).  They can also configure the 
KConfig to not cascade/use the global configuration so they are isolated from 
the environment.
  
  initUrlActionRestrictions can then remain the same, and pass in the 
KConfigGroup that would have been used previously.
  
  It would look something like:
  
KCONFIGCORE_EXPORT void loadUrlActionRestrictions(KConfigGroup cg)
{
// Code as before, without creating the KConfigGroup
}
void initUrlActionRestrictions()
{
KConfigGroup cg(KSharedConfig::openConfig(), "KDE URL Restrictions");
loadUrlActionRestrictions(cg);
}

REPOSITORY
  R237 KConfig

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

To: dfaure, aacid, apol, mdawson
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D22979: Security: remove support for $(...) in config keys with [$e] marker.

[Matthew Dawson]

mdawson accepted this revision.

REPOSITORY
  R237 KConfig

BRANCH
  security_kill_popen

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

To: dfaure, mdawson, aacid, broulik, davidedmundson, kossebau, apol, sitter, 
security-team
Cc: ZaWertun, rikmills, fvogt, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22979: Security: remove support for $(...) in config keys with [$e] marker.

[Matthew Dawson]

mdawson added a comment.


  LGTM.  Regarding the test, if we want to get this change in asap due to the 
security focus I can submit a follow up patch re-adding it.

INLINE COMMENTS

> kconfigtest.cpp:530
>  << "URL[$e]=file://${HOME}/foo" << endl
> -<< "hostname[$e]=$(hostname)" << endl
>  << "escapes=aaa,bb/b,ccc\\,ccc" << endl

Instead of removing this test, can it instead be switched to verify the command 
execution does not occur?

> options.md:78
>  
> -Note that the application will replace `$USER` and `$(hostname)` with their
> +Note that the application will replace `$USER` with their
>  respective expanded values after saving. To prevent this combine the `$e` 
> option

Grammar suggestion: Note that the application will replace `$USER` with its 
expanded values after saving.

REPOSITORY
  R237 KConfig

BRANCH
  security_kill_popen

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

To: dfaure, mdawson, aacid, broulik, davidedmundson, kossebau, apol, sitter, 
security-team
Cc: ngraham, kde-frameworks-devel, LeGast00n, michaelh, bruns

D13947: Honor BUILD_TESTING

[Matthew Dawson]

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


  LGTM, thanks!

REPOSITORY
  R237 KConfig

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

To: arojas, mdawson, dfaure
Cc: asturmlechner, kde-frameworks-devel, michaelh, ngraham, bruns

D6553: Standard shortcuts: use Ctrl+PageUp/PageDown for prev/next tab.

[Matthew Dawson]

mdawson added a comment.


  +1 LGTM.  Before submitting, can you poke the usability people about this 
change please?  If they are also happy, then it's got a ship it from me.

REPOSITORY
  R237 KConfig

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

To: dfaure, mdawson
Cc: ltoscano, apol, #frameworks

D5502: Fix relativePath calculation in KDesktopFile::locateLocal()

[Matthew Dawson]

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


  LGTM!  Thanks for working on this.

REPOSITORY
  R237 KConfig

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

To: wbauer, #frameworks, mdawson
Cc: mdawson, #frameworks

D5502: Fix relativePath calculation in KDesktopFile::locateLocal()

[Matthew Dawson]

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


  Excellent!  Could you add 4 more rows to the tests, to ensure a path without 
a folder (ex. systemConfigLocation + "/test.desktop") works correctly?  Once 
that's done, it's a ship it from me.

REPOSITORY
  R237 KConfig

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

To: wbauer, #frameworks, mdawson
Cc: mdawson, #frameworks

D5502: Fix relativePath calculation in KDesktopFile::locateLocal()

[Matthew Dawson]

mdawson added a comment.


  In https://phabricator.kde.org/D5502#103322, @wbauer wrote:
  
  > In https://phabricator.kde.org/D5502#103316, @mdawson wrote:
  >
  > > Can you please add some unit tests for this, to ensure it doesn't break 
in the future?  I think just three extra tests, one for a desktop file in a 
config directory, one in a data directory, and one present elsewhere would be 
enough.
  >
  >
  > Yes, I'll try.
  >  Though it will take till tomorrow I suppose...
  
  
  No problem, feel free to ask for help!  The tests can go in 
autotests/kdesktopfile.cpp, which already has several tests for KDesktopFile.  
Take a look at the existing tests in that file for an example.

REPOSITORY
  R237 KConfig

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

To: wbauer, #frameworks, mdawson
Cc: mdawson, #frameworks

D5502: Fix relativePath calculation in KDesktopFile::locateLocal()

[Matthew Dawson]

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


  +1 This definitely looks like the correct fix.
  
  Can you please add some unit tests for this, to ensure it doesn't break in 
the future?  I think just three extra tests, one for a desktop file in a config 
directory, one in a data directory, and one present elsewhere would be enough.

REPOSITORY
  R237 KConfig

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

To: wbauer, #frameworks, mdawson
Cc: mdawson, #frameworks

[Differential] [Accepted] D4604: KConfig: stop exporting and installing KConfigBackend.

[Matthew Dawson]

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


  Ship it!

REPOSITORY
  R237 KConfig

BRANCH
  master

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: dfaure, mdawson
Cc: #frameworks

[Differential] [Commented On] D4604: KConfig: stop exporting and installing KConfigBackend.

[Matthew Dawson]

mdawson added inline comments.

INLINE COMMENTS

> dfaure wrote in kconfigbackend.h:210
> In general I agree that leaving dead code is bad.
> 
> In this particular case, though, the other side of the plugin code is still 
> there
> ("#if 0 // TODO port to Qt5 plugin loading" in kconfigbackend.cpp), and 
> actually this is the bit that *was* ported to Qt5 plugin loading, so it will 
> be useful if anyone decides to finally implement plugin based backends. Maybe 
> I can make it
> #if 0 // TODO re-enable if the plugin loading code is re-enabled
> instead of just "//" which is indeed uglier.
> 
> Alternatively, we decide that plugin-based backends are not even on the 
> far-future radar and we clean up both sides completely. Works for me too. The 
> whole idea was crazy anyway (apps make assumptions about config files being 
> files, if one day we change that for e.g. something like gconf then it won't 
> be as transparent as just switching to a different backend).

Sure, #if 0 is best since it has the code, and they will at least match.

I'd actually like to try and bring this back sometime, but as you say it would 
be very difficult.  I don't know if it will ever come to pass, so I don't want 
to hold up actual improvements for a nice to have.  I definitely don't know if 
it would ever be plugin based, or just built into KConfig.

REPOSITORY
  R237 KConfig

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: dfaure, mdawson
Cc: #frameworks

[Differential] [Requested Changes] D4604: KConfig: stop exporting and installing KConfigBackend.

[Matthew Dawson]

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


  I agree, this can't be used so removing it from the API can't hurt anything.  
Just to be sure, let's take this change for the next release, but not change 
KConfigBackend' API/ABI until after the next release.  If anyone complains 
about KConfigBackend missing before then, we can just re-export it easily.  
Otherwise this can always come back in a (ABI) different form if this ever 
becomes worthwhile.  Does that sound good to you?
  
  I do have one small comment before committing.

INLINE COMMENTS

> kconfigbackend.h:210
> +//#define K_EXPORT_KCONFIGBACKEND(libname, classname) \
> +//K_PLUGIN_FACTORY(factory, registerPlugin();)
>  

I don't think this is worth saving as a comment, just remove it completely.  If 
we decide to undo this, it's in git.

REPOSITORY
  R237 KConfig

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: dfaure, mdawson
Cc: #frameworks

Re: KConfig issues prevent compiling KDE applications under Windows

[Matthew Dawson]

On Thursday, February 2, 2017 10:08:53 PM EST Jaroslaw Staniek wrote:
> On 1 February 2017 at 14:34, David Faure  wrote:
> > One note though: this is a failure to link a unittest, your release isn't
> > blocked, you can just disable the building of unittests in kconfig.
> > 
> > The double definition can be explained, the unittest links to
> > KF5ConfigCore
> > and then also compiles in ../src/core/kconfigdata.cpp because that class
> > is not
> > exported.
> 
> Hi,
> Apparently it is since eab822e20620 (Jan 15).
> ​The bug #​375654 does not seem to provide version info but the fix isn't
> just released, right? CC'd Stephen Kelly.
It seems this class was exported so it can be accessed through the Python 
bindings.  Considering it wasn't exported before, I'm wondering why it was 
exported for Python?

Since removing it would be an ABI for Python scripts and the tagging is 
happening today, can we just back out the KEntryMap API from Python?  We can 
add it back for the next release if there is a use case for it.

@David Faure, when do you plan on tagging this change?  For now I committed a 
change to the auto test so it will at least build on Windows 
(b939b48f8d5e5eaf9a51a7e9bda2ad8cedca27d9) which should be included.  Would 
there be time to remove KEntryMap before the tag/release?
-- 
Matthew

signature.asc
Description: This is a digitally signed message part.

[Differential] [Accepted] D3386: Generate an instance with KSharedConfig::Ptr for singleton and arg

[mdawson (Matthew Dawson)]

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


  LGTM!

REPOSITORY
  R237 KConfig

BRANCH
  kconfigcompiler-instance-ksharedconfig

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks, dfaure, mdawson
Cc: ltoscano, aacid, apol

Re: Review Request 129450: Add FreeBSD to metainfo.yaml.

[Matthew Dawson]

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


Ship it!




Ship It!

- Matthew Dawson


On Nov. 20, 2016, 10:40 a.m., Tobias Berner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129450/
> ---
> 
> (Updated Nov. 20, 2016, 10:40 a.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Add FreeBSD to metainfo.yaml.
> 
> 
> Diffs
> -
> 
>   metainfo.yaml cc05742b6689f0d1fbe358665249d13f6e3a6320 
> 
> Diff: https://git.reviewboard.kde.org/r/129450/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tobias Berner
> 
>

Re: Review Request 129453: Add FreeBSD to metainfo.yaml.

[Matthew Dawson]

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


Ship it!




Ship It!

- Matthew Dawson


On Nov. 20, 2016, 10:39 a.m., Tobias Berner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129453/
> ---
> 
> (Updated Nov. 20, 2016, 10:39 a.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kdnssd
> 
> 
> Description
> ---
> 
> Add FreeBSD to metainfo.yaml.
> 
> 
> Diffs
> -
> 
>   metainfo.yaml e64fc5e8d9a3348800e4b74973790e8d58cf1b2d 
> 
> Diff: https://git.reviewboard.kde.org/r/129453/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tobias Berner
> 
>

Re: Review Request 129251: Remove Shift+Del as secondary shortcut for Cut

[Matthew Dawson]

gt; delete on windows, yet on that wikipedia page shift+delete is described as 
> cut shortcut. That is not right!
> 
> Albert Astals Cid wrote:
> > Regarding the shift+delete and wikipedia. I highly doubt what wikipedia 
> says is true. I know with 100% certainty that shift+delete is permanently 
> delete on windows, yet on that wikipedia page shift+delete is described as 
> cut shortcut. That is not right!
> 
> You just made me reboot into windows 10
> 
>  * Open Microsoft Edge
>  * Type "hola" in the url bar
>  * Select it
>  * Press Shift+Delete
>  * Close Microsoft Edge
>  * Open Firefox
>  * Press Ctrl+V
>  * Voilà, "hola" appeared on the url bar of firefox

So I think the best solution going forward is to give primary shortcuts 
precedence over alternate shortcuts.  Then Shift+Delete can be an alternate 
shortcut for Cut and a primary for ForceDelete, which will behave as expected 
in just about every application.  Thoughts?


- Matthew


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


On Oct. 24, 2016, 6:47 a.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129251/
> ---
> 
> (Updated Oct. 24, 2016, 6:47 a.m.)
> 
> 
> Review request for KDE Frameworks, KDE Usability and Matthew Dawson.
> 
> 
> Bugs: 357747
> https://bugs.kde.org/show_bug.cgi?id=357747
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> This patch removes Shift+Del as secondary shortcut for the Cut action. This 
> shortcut was set back in 2001.
> 
> Reasons for removing it:
> 
> * The expected standard behavior for this shortcut is "Permanently delete"
> * For the reason above, it is also set as primary shortcut for the 
> `DeleteFile` action. This causes conflicts in applications.
> * For the reason above, many applications (e.g. Dolphin or Digikam) already 
> resolve this conflict on their own.
> 
> Credits to Jan for the investigation: 
> https://bugs.kde.org/show_bug.cgi?id=347373#c2
> 
> 
> Diffs
> -
> 
>   src/gui/kstandardshortcut.cpp 92eb091382c7ab2110240cef21f29268be787250 
> 
> Diff: https://git.reviewboard.kde.org/r/129251/diff/
> 
> 
> Testing
> ---
> 
> Using Shift+Del in Gwenview now works as expected.
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

Re: Review Request 128608: Add Donate entry to KStandardShortcut

[Matthew Dawson]

> On Aug. 9, 2016, 11:23 p.m., Matthew Dawson wrote:
> > LGTM.  I can't speak for the other review.

Sorry, one other thing:  Please make sure to put a CHANGELOG: entry in the 
commit message.  Thanks!


- Matthew


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


On Aug. 8, 2016, 5:05 a.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128608/
> ---
> 
> (Updated Aug. 8, 2016, 5:05 a.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> This patch adds a Donate entry, such that 
> https://git.reviewboard.kde.org/r/128609/ builds.
> 
> There is no default shortcut set.
> 
> 
> Diffs
> -
> 
>   src/gui/kstandardshortcut.h a5a7c28f86094bd346abc8dc17e24a6bd365f5bd 
>   src/gui/kstandardshortcut.cpp 6d749455782d55a81ed5448f35eade954c1df615 
> 
> Diff: https://git.reviewboard.kde.org/r/128608/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

Re: Review Request 128608: Add Donate entry to KStandardShortcut

[Matthew Dawson]

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


Ship it!




LGTM.  I can't speak for the other review.

- Matthew Dawson


On Aug. 8, 2016, 5:05 a.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128608/
> ---
> 
> (Updated Aug. 8, 2016, 5:05 a.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> This patch adds a Donate entry, such that 
> https://git.reviewboard.kde.org/r/128609/ builds.
> 
> There is no default shortcut set.
> 
> 
> Diffs
> -
> 
>   src/gui/kstandardshortcut.h a5a7c28f86094bd346abc8dc17e24a6bd365f5bd 
>   src/gui/kstandardshortcut.cpp 6d749455782d55a81ed5448f35eade954c1df615 
> 
> Diff: https://git.reviewboard.kde.org/r/128608/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

Re: Review Request 128019: Remove F1 as standard shortcut for Help

[Matthew Dawson]

> On May 26, 2016, 11:37 a.m., Matthew Dawson wrote:
> > While I appreciate the source of this change (I've almost never hit F1 
> > intending to open the doucmentation myself), I don't think a RR is the 
> > right place to discuss changes to our default shortcuts.  This takes a 
> > small survey of a small section of our community.  I think the VDG would 
> > probably be the best place to have a discussion about what help, if any, is 
> > triggered and on what shortcut (though if there is a better source I'm 
> > happy to take their input instead).  Once there is a consensus, then I'm 
> > happy to have those changes implemented.
> > 
> > Since there is a global way to disable F1 for people who find this annoying 
> > now, this is a -2 from me.  People annoyed at F1 can fix their issue now 
> > without code changes while we figure out the best plan forward.
> 
> Albert Astals Cid wrote:
> The proper way of doing this is having metrics reporting that show how 
> quickly after pressing F1 you close khelpcenter, unfortunately the user 
> metric reporting got blocked on "this is spying our users", so now we don't 
> have any data that can back my claim that F1 as a shortcut is useless or 
> someone else's claim that it is vital.
> 
> I will discard this and stop pursuing the idea, it's clear we're 
> stationary and people are scared of change or even getting the data to pursue 
> change.
> 
> Matthew Dawson wrote:
> To be clear, I'm not against this change, I'm just against having the 
> conversation about this change in *this* communication channel.  Like I said, 
> if this is taken to something like VDG, and the consensus there is to remove 
> this shortcut, I'll happily take this (even if there are some objections).
> 
> Please don't stop pursuing this!  I agree the current behaviour is 
> suboptimal and something new should be found.  My goal isn't to be stationary.
> 
> Albert Astals Cid wrote:
> The kde-usability mailing list is subscribed here.  In my opinion that's 
> enough to get usability people to comment, your -2 means you think it is not.
> 
> I do not have time to do more than what i have done, i have 29 okular 
> review requests pending and i should really get to review them this century, 
> so no, i'm not going to pursue this further.
> 
> If you think the current behavior is suboptimal maybe you can be 
> convinced to do it ;)

I don't find RR are a great place for this type of conversation, especially 
something that may take a wider vision.  If kde-usability comes to a consensus, 
that's good enough for me.

Fair enough on your time constraints, I use okular all the time and appreciate 
your work in it :).  I'll see what I can do about it, though I have the same 
time problems.


- Matthew


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


On May 26, 2016, 6:04 p.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128019/
> ---
> 
> (Updated May 26, 2016, 6:04 p.m.)
> 
> 
> Review request for KDE Frameworks and KDE Usability.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> F1 is too important and too easy to trigger for something like Help, that be 
> honest you don't need a shortcut for (since you don't invoke Help that often).
> 
> 
> Diffs
> -
> 
>   src/gui/kstandardshortcut.cpp 6be6309 
> 
> Diff: https://git.reviewboard.kde.org/r/128019/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

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

Re: Review Request 128019: Remove F1 as standard shortcut for Help

[Matthew Dawson]

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



While I appreciate the source of this change (I've almost never hit F1 
intending to open the doucmentation myself), I don't think a RR is the right 
place to discuss changes to our default shortcuts.  This takes a small survey 
of a small section of our community.  I think the VDG would probably be the 
best place to have a discussion about what help, if any, is triggered and on 
what shortcut (though if there is a better source I'm happy to take their input 
instead).  Once there is a consensus, then I'm happy to have those changes 
implemented.

Since there is a global way to disable F1 for people who find this annoying 
now, this is a -2 from me.  People annoyed at F1 can fix their issue now 
without code changes while we figure out the best plan forward.

- Matthew Dawson


On May 25, 2016, 7:01 p.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128019/
> ---
> 
> (Updated May 25, 2016, 7:01 p.m.)
> 
> 
> Review request for KDE Frameworks and KDE Usability.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> F1 is too important and too easy to trigger for something like Help, that be 
> honest you don't need a shortcut for (since you don't invoke Help that often).
> 
> 
> Diffs
> -
> 
>   src/gui/kstandardshortcut.cpp 6be6309 
> 
> Diff: https://git.reviewboard.kde.org/r/128019/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

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

Re: Review Request 127948: Avoid skipping KAuthorized check

[Matthew Dawson]

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


Fix it, then Ship it!




Excellent, thanks!  Just one comment below about the name of the test.  If that 
name is good with you, feel free to change + push.


autotests/kdesktopfiletest.h (line 36)
<https://git.reviewboard.kde.org/r/127948/#comment64774>

Can you change this name to ```testTryExecWithAuthorizeAction```?  
testTryExec sound like it is testing TryExec in general, which is being tested 
above.  That name is hopefully a little more descriptive of the issue under 
test.


- Matthew Dawson


On May 19, 2016, 4:17 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127948/
> ---
> 
> (Updated May 19, 2016, 4:17 p.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Previously, if a .desktop file had a TryExec and a X-KDE-AuthorizeAction
> entry we would skip the KAuthorized check. This is clearly wrong.
> 
> 
> Diffs
> -
> 
>   autotests/kdesktopfiletest.h f4e0c96b8751b2b116b98a52553ad6698b9d40b4 
>   autotests/kdesktopfiletest.cpp a90faf3ca44b6a8f5d7d7c9de4d58d17ba360d16 
>   src/core/kdesktopfile.cpp eda7c292da9aa2bc97f6066e90034767134c2e07 
> 
> Diff: https://git.reviewboard.kde.org/r/127948/diff/
> 
> 
> Testing
> ---
> 
> Disabled shell_access in kde5rc.
> 
> With I was unable to open konsole from kicker.
> Opening dolphin still worked as normal.
> 
> 
> 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 127948: Avoid skipping KAuthorized check

[Matthew Dawson]

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



+1 from me.  Before pushing, can you please add a unit test for this behaviour? 
 Otherwise looks good to go.

- Matthew Dawson


On May 17, 2016, 3:57 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127948/
> ---
> 
> (Updated May 17, 2016, 3:57 p.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Previously, if a .desktop file had a TryExec and a X-KDE-AuthorizeAction
> entry we would skip the KAuthorized check. This is clearly wrong.
> 
> 
> Diffs
> -
> 
>   src/core/kdesktopfile.cpp eda7c292da9aa2bc97f6066e90034767134c2e07 
> 
> Diff: https://git.reviewboard.kde.org/r/127948/diff/
> 
> 
> Testing
> ---
> 
> Disabled shell_access in kde5rc.
> 
> With I was unable to open konsole from kicker.
> Opening dolphin still worked as normal.
> 
> 
> 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 127813: Process paths just once

[Matthew Dawson]

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



General +1 from me, just two things:

1) Can you split these two change into two commits?  One being the part about 
not removing file: twice, the second about removing duplicate homes?  If not, 
np.
2) Do you have any benchmark numbers about this?  Just looking at comparisions, 
it will take three comparisions each time to check the list, then at least one 
more comparision assuming they are all the same.  Versus only three 
comparisions on the old code.  And if any path does clean the string, the other 
comparisions are skipped in the old version.  That being said, if benchmarks 
suggest your version is faster in wall time (not callgrind counts), I have no 
problem taking it.

- Matthew Dawson


On May 4, 2016, 9:02 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127813/
> ---
> 
> (Updated May 4, 2016, 9:02 p.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Just process every home path once, as the 3 alternatives will probably just 
> be the same.
> Don't drop the file: prefix twice in every run.
> 
> 
> Diffs
> -
> 
>   src/core/kconfiggroup.cpp 39d2441 
> 
> Diff: https://git.reviewboard.kde.org/r/127813/diff/
> 
> 
> Testing
> ---
> 
> Tests pass, apps work.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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

Re: Review Request 127765: RFC: Cache global config files

[Matthew Dawson]

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



+1 on the concept.  Making things faster for everyone for free is good by me.  
But, since it is an optimization we just need to make sure it doesn't regress 
KConfig.  Albert's point about caching even with changes definitely needs 
fixing (I don't think a stat call here should negatively affect anything), and 
I won't be able to review this properly until this weekend.  So this isn't a 
shipit yet, but I definitely want it in.

One thing to look at, could this be extended to all config files?  So if 
someone just keeps opening the same config, there is less of a penalty?  If 
not, that's fine.  Ideally they wouldn't do that in the first place anyways.

- Matthew Dawson


On April 27, 2016, 12:14 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127765/
> ---
> 
> (Updated April 27, 2016, 12:14 p.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> A next step for my little quest is improving KConfig impact upon start.
> 
> In callgrind terms, 20% of dolphin's startup time is KConfig and 15% is 
> parsing global files, which is essentially loading kdeglobals 70 times. This 
> of course also means that kdeglobals is scattered 70 times in each 
> application's memory space.
> 
> To improve such situation, here's an attempt to cache these. I'm not an 
> expert in KConfig, so feedback is really appreciated 
> [[1]](http://i1.kym-cdn.com/photos/images/facebook/000/234/765/b7e.jpg)
> 
> 
> Diffs
> -
> 
>   src/core/kconfig.cpp ad52da9 
> 
> Diff: https://git.reviewboard.kde.org/r/127765/diff/
> 
> 
> Testing
> ---
> 
> Tests pass, KConfig becomes 6% of dolphin at load.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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

Re: Review Request 127462: Add support for get QStandardPaths locations.

[Matthew Dawson]

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



LGTM.  Two minor issues with the documentation, then it is good to go in.  
Please also add a CHANGELOG line to the commit message.  Thanks for taking care 
of this.


docs/options.md (line 94)
<https://git.reviewboard.kde.org/r/127462/#comment64058>

Grammer nitpick, maybe instead:

There are three environment variables that have a fallback strategy if the
environment variable is not set. They instead map to a location from 
QStanardPaths.  They are:



docs/options.md (line 96)
<https://git.reviewboard.kde.org/r/127462/#comment64059>

Nitpick: s/QStandardsPath/QStandardPaths/


- Matthew Dawson


On March 28, 2016, 11:21 a.m., Sandro Knauß wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127462/
> ---
> 
> (Updated March 28, 2016, 11:21 a.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Inside desktop files we want to reach also data, cache and config home
> to create files inside these directories.
> 
> 
> Diffs
> -
> 
>   autotests/kconfigtest.h be0a17ea66fbca989a53c68481c4252c9546dd45 
>   autotests/kconfigtest.cpp e92197f3be57ead47b70ca5d040474e7a554c416 
>   docs/options.md c7a6c061b700fd7a23b5dd1628cd22a18dec79da 
>   src/core/kconfig.cpp 07fa6f552c61c52cc1dd64a1c5fb0e2f00873d50 
> 
> Diff: https://git.reviewboard.kde.org/r/127462/diff/
> 
> 
> Testing
> ---
> 
> Adding tests for QT_*_HOME variables.
> 
> 
> Thanks,
> 
> Sandro Knauß
> 
>

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

Re: Review Request 127462: Add support for get QStandardPaths locations.

[Matthew Dawson]

> On March 28, 2016, 1:39 a.m., Matthew Dawson wrote:
> > Much better!  I don't think this should handle XDG* variables explicitly, 
> > as they won't be used on other platforms and may cause confusion there.  On 
> > platforms using XDG* variables, Qt handles this for us internally.  I 
> > opened issues accordingly below.  Other then that, looks good to me.
> 
> Sandro Knauß wrote:
> But where does Qt handles these internally? At the moment Qt isn't doing 
> this at all. If we don't respect XDG_*_Home it is a unexpected result for XDG 
> conform desktops. Do you suggest to #ifdef the XDG handling?

In the qtbase repository, look at the src/corelib/io/qstandardpaths_unix.cpp 
file (http://osxr.org/qt/source/qtbase/src/corelib/io/qstandardpaths_unix.cpp, 
line 84 for instance.  Also in git).  It returns the path for QStandardPaths, 
and checks the value of the XDG variables.  Considering there is lots of other 
software that relies on this functionality, either everything else in the Qt 
world is not XDG compliant, or Qt handles it for us.


> On March 28, 2016, 1:39 a.m., Matthew Dawson wrote:
> > autotests/kconfigtest.cpp, line 581
> > <https://git.reviewboard.kde.org/r/127462/diff/2/?file=454401#file454401line581>
> >
> > We unfortunately can't test XDG* variables due to Qt, but I think we'll 
> > be ok for now.  The rest of the looks fine.
> 
> Sandro Knauß wrote:
> Well the test is green, so this works :D

Right, but if/when you remove the explicit variable handling below, it will 
break.


- Matthew


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


On March 27, 2016, 10:22 a.m., Sandro Knauß wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127462/
> -----------
> 
> (Updated March 27, 2016, 10:22 a.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Inside desktop files we want to reach also data, cache and config home
> to create files inside these directories.
> 
> 
> Diffs
> -
> 
>   autotests/kconfigtest.h be0a17ea66fbca989a53c68481c4252c9546dd45 
>   autotests/kconfigtest.cpp e92197f3be57ead47b70ca5d040474e7a554c416 
>   docs/options.md c7a6c061b700fd7a23b5dd1628cd22a18dec79da 
>   src/core/kconfig.cpp 07fa6f552c61c52cc1dd64a1c5fb0e2f00873d50 
> 
> Diff: https://git.reviewboard.kde.org/r/127462/diff/
> 
> 
> Testing
> ---
> 
> Adding tests for XDG_*_HOME variables.
> 
> 
> Thanks,
> 
> Sandro Knauß
> 
>

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

Re: Review Request 127462: Add support for get QStandardPaths locations.

[Matthew Dawson]

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



Much better!  I don't think this should handle XDG* variables explicitly, as 
they won't be used on other platforms and may cause confusion there.  On 
platforms using XDG* variables, Qt handles this for us internally.  I opened 
issues accordingly below.  Other then that, looks good to me.


autotests/kconfigtest.cpp (line 581)
<https://git.reviewboard.kde.org/r/127462/#comment64047>

We unfortunately can't test XDG* variables due to Qt, but I think we'll be 
ok for now.  The rest of the looks fine.



docs/options.md (line 94)
<https://git.reviewboard.kde.org/r/127462/#comment64046>

Nitpick: please don't use wildcards here, as it isn't clear to people what 
variables we are talking about.  Instead just point to the list below.

Also, since XDG_*_HOME variables won't be handled on other platforms, we 
don't need to mention it here.



docs/options.md (line 98)
<https://git.reviewboard.kde.org/r/127462/#comment64049>

I think it be enough to just refer to the enum variable here (ex 
QStandardsPath::GenericConfigLocation instead of 
QStandardsPath::writeableLocation(GenericConfigLocation)).



docs/options.md (line 102)
<https://git.reviewboard.kde.org/r/127462/#comment64048>

I think this paragraph can be removed if XDG* isn't handled explicitly.



src/core/kconfig.cpp (line 226)
<https://git.reviewboard.kde.org/r/127462/#comment64045>

This should be handled internally by Qt, so we don't have to explicitly 
handle this case.

On non-unix platforms, we shouldn't be using those variables as they have 
their own standard we should integrate against.


- Matthew Dawson


On March 27, 2016, 10:22 a.m., Sandro Knauß wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127462/
> ---
> 
> (Updated March 27, 2016, 10:22 a.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Inside desktop files we want to reach also data, cache and config home
> to create files inside these directories.
> 
> 
> Diffs
> -
> 
>   autotests/kconfigtest.h be0a17ea66fbca989a53c68481c4252c9546dd45 
>   autotests/kconfigtest.cpp e92197f3be57ead47b70ca5d040474e7a554c416 
>   docs/options.md c7a6c061b700fd7a23b5dd1628cd22a18dec79da 
>   src/core/kconfig.cpp 07fa6f552c61c52cc1dd64a1c5fb0e2f00873d50 
> 
> Diff: https://git.reviewboard.kde.org/r/127462/diff/
> 
> 
> Testing
> ---
> 
> Adding tests for XDG_*_HOME variables.
> 
> 
> Thanks,
> 
> Sandro Knauß
> 
>

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

Re: Review Request 127462: Add support for XDG_*_HOME enviroment variables.

[Matthew Dawson]

be the interface for config files to the QStandardPaths API.

+1 from me on this suggestion.  I'm good with this change if the names are 
changed and David's other points are addressed.


- Matthew


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


On March 22, 2016, 11:23 a.m., Sandro Knauß wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127462/
> ---
> 
> (Updated March 22, 2016, 11:23 a.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> According to freedesktop specification XDG_*_HOME env varaible should be
> replaced, if they are not setted with default values.
> 
> https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html
> 
> as qgetenv only calls getenv, so no path is traslated to it default values. 
> So we have to add this replacement manually. This would help to use 
> XDG_*_HOME more often in configfiles.
> 
> 
> Diffs
> -
> 
>   autotests/kconfigtest.cpp e92197f3be57ead47b70ca5d040474e7a554c416 
>   src/core/kconfig.cpp 07fa6f552c61c52cc1dd64a1c5fb0e2f00873d50 
> 
> Diff: https://git.reviewboard.kde.org/r/127462/diff/
> 
> 
> Testing
> ---
> 
> Adding tests for XDG_*_HOME variables.
> 
> 
> Thanks,
> 
> Sandro Knauß
> 
>

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

Re: kconfig question

[Matthew Dawson]

On Wednesday, January 27, 2016 10:32:50 AM EST Boudewijn Rempt wrote:
> On Sat, 23 Jan 2016, David Faure wrote:
> >  (I don't mean "it's called KConfig" ;-) I mean what does the code using
> >  it, look like? I think this needs some debugging to find out where this
> >  "local" string comes from)
> Well, it's done in a lot of places, about 160, like:
> 
>   KConfigGroup cfg = 
> KSharedConfig::openConfig()->group("advancedColorSelector");
> 
> or
> 
>   KConfigGroup cg(KSharedConfig::openConfig(), "");
> 
> 
> What I want in the end is have AppData\Roaming\Krita\ where both kritarc and
> the user's custom resources are saved, like brushes and so on. I'm fine
> with patching kconfig for windows for that, though.
Won't this change the behaviour from Unix like systems?  They won't have the 
Krita folder in the name, so they will just get dumped in the common config 
folder.  I don't think carrying a patch like that for KConfig is a good idea.
-- 
Matthew

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: kconfig question

[Matthew Dawson]

On Saturday, January 23, 2016 11:54:56 AM EST David Faure wrote:
> On Monday 18 January 2016 10:28:18 Matthew Dawson wrote:
> > On Monday, January 18, 2016 8:53:22 AM EST Boudewijn Rempt wrote:
> > > On Sun, 17 Jan 2016, Matthew Dawson wrote:
> > > > It appears this is the desired behaviour.  On most platforms,
> > > > GenericConfigLocation and ConfigLocation are the same things, except
> > > > Windows. According to Qt's history, ConfigLocation was supposed to act
> > > > like GenericConfigLocation, but a mistake was made and
> > > > GenericConfigLocation was introduced to fix that without breaking
> > > > existing applications.  See commit
> > > > 44d48862c0ff4b67a76734deae5e76f926a77bce .
> > > > 
> > > > KConfig was then changed to use GenericConfigLocation as well, see
> > > > commit
> > > > b3487a0684fa464ada0cd3407e96d0406345a42a in kdelibs (this happened
> > > > before
> > > > the split).  On all platforms except Windows, that is how KConfig has
> > > > all
> > > > behaved. Even in KDE4, config files were often put in the same
> > > > directory,
> > > > just not a standard directory shared by the platform.
> > > > 
> > > > Based on the commit history, I'd say it makes sense.  Anything else
> > > > would
> > > > yield platform inconsistent behaviour, which is more likely to confuse
> > > > people. If ConfigLocation wasn't inconsistent across platforms, that
> > > > would be a different story.
> > > 
> > > Hm, but I don't think that having kritarc in a directory called "local"
> > > on
> > > every platform is important: the important thing is to have it in the
> > > location that's "normal" on those platforms.
> > 
> > Taking a quick look at my Win7 machine suggests that the path Qt uses for
> > GenericConfigLocation is a mix of .config/.local/.cache on *nix. 
> > According to Qt's documents, no other system uses a local/ folder for
> > GenericConfigLocation.  So that seems "normal".
> 
> Not sure this answers Boud's question, since he *is* seeing "Local" on
> Windows. He said: "I noticed that krita on windows wrote its kritarc to
> Roaming\local\ or Local\local "
> 
> According to the QStandardPaths documentation, it is expected to see
> something like "C:/Users//AppData/Local" in the paths for
> GenericConfigLocation
> 
> The the lowercase "local" after that puzzles me. I wonder where that comes
> from, it's not anywhere in the QStandardPaths code for Windows, nor KConfig
> AFAICS.
I thought he mean Romaing\ or Local\ to be the path to AppData, where roaming 
or local changed based upon how the Windows profile was setup.  The second 
local\ I had assumed was coming from Qt, since it appeared in both.

If that was not the intent, then I apologize and agree with David.  If that is 
the case, I'll try and do some digging.  I just don't have a Windows machine 
handy I can do this on.

> 
> Boud, how is KConfig called exactly?
>   (I don't mean "it's called KConfig" ;-) I mean what does the code using
> it, look like? I think this needs some debugging to find out where this
> "local" string comes from)
> > > > Regardless, changing the default behaviour now is a no go, as that
> > > > would
> > > > break everyone's setup.  However, I wouldn't be opposed to a method to
> > > > allow KConfig to easily store config files in an application
> > > > appropriate
> > > > folder.  It couldn't break existing applications though.  I also don't
> > > > think most things should need it anyways, since most applications only
> > > > have one config file. Something like akonadi (which already uses a
> > > > subfolder in my .config) makes sense, and I could see it making sense
> > > > for
> > > > Krita.  Maybe some way to use AppConfigLocation?  We'd have to be
> > > > careful
> > > > with that though, since it's>
> > > > 
> > > > >=5.5.
> > > 
> > > I'm fine with 5.5 as a minimum for my own purposes :-). I just don't
> > > want to have to change a couple of hundred locations to explicitly pass
> > > AppConfigLocation.
> > 
> > I meant that a patch for KConfig would have to be careful, since KConfig
> > can't depend on 5.5 (yet).  I think just using the defaults will be ok,
> > if you have only one config file.  If you have more, then I'd agree that
> > AppConfigLocatio

Re: kconfig question

[Matthew Dawson]

On Monday, January 18, 2016 8:53:22 AM EST Boudewijn Rempt wrote:
> On Sun, 17 Jan 2016, Matthew Dawson wrote:
> > It appears this is the desired behaviour.  On most platforms,
> > GenericConfigLocation and ConfigLocation are the same things, except
> > Windows. According to Qt's history, ConfigLocation was supposed to act
> > like GenericConfigLocation, but a mistake was made and
> > GenericConfigLocation was introduced to fix that without breaking
> > existing applications.  See commit
> > 44d48862c0ff4b67a76734deae5e76f926a77bce .
> > 
> > KConfig was then changed to use GenericConfigLocation as well, see commit
> > b3487a0684fa464ada0cd3407e96d0406345a42a in kdelibs (this happened before
> > the split).  On all platforms except Windows, that is how KConfig has all
> > behaved. Even in KDE4, config files were often put in the same directory,
> > just not a standard directory shared by the platform.
> > 
> > Based on the commit history, I'd say it makes sense.  Anything else would
> > yield platform inconsistent behaviour, which is more likely to confuse
> > people. If ConfigLocation wasn't inconsistent across platforms, that
> > would be a different story.
> 
> Hm, but I don't think that having kritarc in a directory called "local" on
> every platform is important: the important thing is to have it in the
> location that's "normal" on those platforms.
Taking a quick look at my Win7 machine suggests that the path Qt uses for 
GenericConfigLocation is a mix of .config/.local/.cache on *nix.  According to 
Qt's documents, no other system uses a local/ folder for 
GenericConfigLocation.  So that seems "normal".

> 
> > Regardless, changing the default behaviour now is a no go, as that would
> > break everyone's setup.  However, I wouldn't be opposed to a method to
> > allow KConfig to easily store config files in an application appropriate
> > folder.  It couldn't break existing applications though.  I also don't
> > think most things should need it anyways, since most applications only
> > have one config file. Something like akonadi (which already uses a
> > subfolder in my .config) makes sense, and I could see it making sense for
> > Krita.  Maybe some way to use AppConfigLocation?  We'd have to be careful
> > with that though, since it's> 
> > >=5.5.
> 
> I'm fine with 5.5 as a minimum for my own purposes :-). I just don't want to
> have to change a couple of hundred locations to explicitly pass
> AppConfigLocation.
I meant that a patch for KConfig would have to be careful, since KConfig can't 
depend on 5.5 (yet).  I think just using the defaults will be ok, if you have 
only one config file.  If you have more, then I'd agree that AppConfigLocation 
seems better.

-- 
Matthew

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: kconfig question

[Matthew Dawson]

On Friday, January 15, 2016 9:46:00 AM EST Boudewijn Rempt wrote:
> I noticed that krita on windows wrote its kritarc to Roaming\local\ or
> Local\local instead of Roaming\krita\kritarc, and I was wondering why that
> was. I now think it's because KConfig's default is to write to
> GenericConfigLocation instead of ConfigLocation.
> 
> Is that really intended to be the default for kconfig? I guess it's wrong
> for pretty much 100% of applications that use kconfig, because pretty much
> every application should write its config to ConfigLocation instead.
> 
> There are a couple of hundred places in Krita where a config object is
> retrieved and before I start adapting all those by adding two extra
> parameter (or writing a wrapper around kconfig, or something else...) I'd
> like to know why GenericConfigLocation is the default :-)
It appears this is the desired behaviour.  On most platforms, 
GenericConfigLocation and ConfigLocation are the same things, except Windows.  
According to Qt's history, ConfigLocation was supposed to act like 
GenericConfigLocation, but a mistake was made and GenericConfigLocation was 
introduced to fix that without breaking existing applications.  See commit 
44d48862c0ff4b67a76734deae5e76f926a77bce .

KConfig was then changed to use GenericConfigLocation as well, see commit 
b3487a0684fa464ada0cd3407e96d0406345a42a in kdelibs (this happened before the 
split).  On all platforms except Windows, that is how KConfig has all behaved.  
Even in KDE4, config files were often put in the same directory, just not a 
standard directory shared by the platform.

Based on the commit history, I'd say it makes sense.  Anything else would 
yield platform inconsistent behaviour, which is more likely to confuse people.  
If ConfigLocation wasn't inconsistent across platforms, that would be a 
different story.

Regardless, changing the default behaviour now is a no go, as that would break 
everyone's setup.  However, I wouldn't be opposed to a method to allow KConfig 
to easily store config files in an application appropriate folder.  It 
couldn't break existing applications though.  I also don't think most things 
should need it anyways, since most applications only have one config file.  
Something like akonadi (which already uses a subfolder in my .config) makes 
sense, and I could see it making sense for Krita.  Maybe some way to use 
AppConfigLocation?  We'd have to be careful with that though, since it's 
>=5.5.
-- 
Matthew

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: Review Request 126556: Ensure group is unescaped properly in kconf_update.

[Matthew Dawson]

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

(Updated Dec. 30, 2015, 12:35 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Matthew Dawson.


Changes
---

Submitted with commit 55bf90ff787f4e84947a5df833f66082e54016df by Matthew 
Dawson to branch master.


Repository: kconfig


Description
---

During a kconf_update run, an invalid group name may be treated as correct
even though the name failed to unescape.  This leads the group name to be
a null character, which will fail.

If the unescape failed, return that failure status instead.  This should have
no impact, as the previous result would have been wrong anyways.  Now a
more useful diagnostic will be returned instead.

Update the unit tests to ensure this issue is tested in the future.

Found in Coverity issue 258087.


Diffs
-

  autotests/test_kconfigutils.cpp 0946cf8a89135e12c1769a3ab6749434aa9691ce 
  src/kconf_update/kconfigutils.cpp 02b3f0a9d245fdc9d0067635e9b59bc84eefc206 

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


Testing
---

Unit tests pass after change, new test fails without code change.


Thanks,

Matthew Dawson

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

Re: Review Request 126555: Remove unused variable in KConfigPrivate

[Matthew Dawson]

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

(Updated Dec. 30, 2015, 12:38 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Matthew Dawson.


Changes
---

Submitted with commit df3440f328405623de927595b4555e06600d5626 by Matthew 
Dawson to branch master.


Repository: kconfig


Description
---

This seems to be left over from KDE 3.5 times, and isn't referenced anywhere
in the code.  Since its a private header, just remove.

Found by Coverity, issue 1175531.


Diffs
-

  src/core/kconfig_p.h b93c8167bbab1d0403493505a5639524b4737f3c 

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


Testing
---

Compiles, test run.  Grepped source code for references, none found.


Thanks,

Matthew Dawson

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

Review Request 126555: Remove unused variable in KConfigPrivate

[Matthew Dawson]

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

Review request for KDE Frameworks and Matthew Dawson.


Repository: kconfig


Description
---

This seems to be left over from KDE 3.5 times, and isn't referenced anywhere
in the code.  Since its a private header, just remove.

Found by Coverity, issue 1175531.


Diffs
-

  src/core/kconfig_p.h b93c8167bbab1d0403493505a5639524b4737f3c 

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


Testing
---

Compiles, test run.  Grepped source code for references, none found.


Thanks,

Matthew Dawson

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

Review Request 126556: Ensure group is unescaped properly in kconf_update.

[Matthew Dawson]

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

Review request for KDE Frameworks and Matthew Dawson.


Repository: kconfig


Description
---

During a kconf_update run, an invalid group name may be treated as correct
even though the name failed to unescape.  This leads the group name to be
a null character, which will fail.

If the unescape failed, return that failure status instead.  This should have
no impact, as the previous result would have been wrong anyways.  Now a
more useful diagnostic will be returned instead.

Update the unit tests to ensure this issue is tested in the future.

Found in Coverity issue 258087.


Diffs
-

  autotests/test_kconfigutils.cpp 0946cf8a89135e12c1769a3ab6749434aa9691ce 
  src/kconf_update/kconfigutils.cpp 02b3f0a9d245fdc9d0067635e9b59bc84eefc206 

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


Testing
---

Unit tests pass after change, new test fails without code change.


Thanks,

Matthew Dawson

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

Re: Review Request 126161: OS X housekeeping

[Matthew Dawson]

On November 27, 2015 01:02:31 PM Alexander Richardson wrote:
> Has anyone done measurements on a recent system? Does it give any
> noticable benefit?
I have not, nor are my machines good representations.  I don't think 
considering machines with 2G of memory with older processors to be out of line 
of what KDE wants to support.

I don't know if it is still true, but I seem to remember the official NVidia 
drivers had a non-pic libGL, which this system made less painful for the 
system.  I also don't use that driver, so I can't comment on today's reality.

> Because there is definitively a security implication of this as it
> completely breaks ASLR.
I don't believe this completely breaks ASLR, at least no more then a regular 
run of prelink will.

When kdeinit first launches, it is still subject to ASLR, which will randomize 
the location of libraries.  Reusing these locations for all applications 
doesn't fully break ASLR, as it is unknown outside the system where an 
application launches.  Thus for an attacker going after one application, they 
are no further ahead then if you disable this.
It does weaken it some, as once you get the layout of one kdeinit launched 
application, you know locations for every other application in that login 
session.  So if an exploit path involves crossing multiple KDE applications 
using ROP, it will be easier to setup.  Though considering how KDE 
applications currently work, I'm not sure that matters as we don't use 
sandboxes.

So, if this optimization isn't useful anymore, I'm all for removing.  But it 
isn't a major security issue so we shouldn't remove it as a knee jerk 
reaction.
-- 
Matthew

smime.p7s
Description: S/MIME cryptographic signature
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126001: In KConfigTest::testEntryMap, convert QByteArray with nulls using a char *

[Matthew Dawson]

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

(Updated Nov. 9, 2015, 2:43 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Changes
---

Submitted with commit 0f1e67051dae0c9b1f23461f7caed594795e8a27 by Matthew 
Dawson to branch master.


Repository: kconfig


Description
---

Due to https://codereview.qt-project.org/#/c/106473/, Qt 5.6 keeps null
characters in QByteArray -> QString conversions, which breaks this test as
one QByteArray contains nulls.  Instead, convert the QByteArray to const
char * first, so QString stops at the first null.

The actual  behaviour of KConfig is unchanged, as internally the conversion
always went through a const char *, which avoids creating QStrings with
null characters.


Diffs
-

  autotests/kconfigtest.cpp 9a2998647b5e5f54d63059172b727505a8ae1c80 

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


Testing
---

Tested on both Qt 5.5.1 and Qt 5.6 (commit 
e996d68f6130847637ba287518cff1289cfa48e5), tests all pass now.


Thanks,

Matthew Dawson

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

Re: Review Request 126001: In KConfigTest::testEntryMap, convert QByteArray with nulls using a char *

[Matthew Dawson]

> On Nov. 9, 2015, 2:57 a.m., David Faure wrote:
> > Isn't this exactly the wrong fix? The goal of the test with embedded NULs 
> > in a QByteArray is to test that KConfig can be used to store binary data, 
> > isn't it? So I would think the expected value was correct, it's the KConfig 
> > code that is faulty. As a programmer I expect to be able to write binary 
> > data and read it back again, without it stopping at the first \0.

I'm not exactly sure what this test is trying to do, or what the purpose of the 
KConfig::entryMap function is.  KConfig::entryMap only returns QString (not 
QByteArray, or any other type), so its value seems limited when I can get the 
same behaviour through other methods.  There is a test that looks at the raw 
byte string to ensure nulls are passed through correctly (see kconfigtest.cpp, 
line 343), so changing the behaviour just to test nulls isn't useful.

What I'm not sure of is what someone would expect when asking for a QString 
from a value in KConfig containing nulls.  If someone is thinking like a C 
programmer, I'd expect them to expect the string to end at the null character.  
Otherwise, I can see the argument returning the string with nulls embedded in 
at as being the expected outcome.  I think either way, people will end up 
suprised.

I'd also expect the KConfigGroup::readEntry to return the same value as what 
KConfig::entryMap returns.  Currently readEntry also stops at the first null 
character.

I'm not aware of a preference in the Qt/KF5 world of how to deal with nulls in 
a string.  If there is a preference either way, I'm happy to take that to fit 
in with the rest of the ecosystem.


Note: none of this changes how a QByteArray should work, as I'd expect that to 
return the exact binary data you feed into it.  If it stopped processing at the 
first 0 byte, I'd consider that to be a bug regardless of how QString is 
handled.


- Matthew


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


On Nov. 8, 2015, 7:23 p.m., Matthew Dawson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126001/
> ---
> 
> (Updated Nov. 8, 2015, 7:23 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Due to https://codereview.qt-project.org/#/c/106473/, Qt 5.6 keeps null
> characters in QByteArray -> QString conversions, which breaks this test as
> one QByteArray contains nulls.  Instead, convert the QByteArray to const
> char * first, so QString stops at the first null.
> 
> The actual  behaviour of KConfig is unchanged, as internally the conversion
> always went through a const char *, which avoids creating QStrings with
> null characters.
> 
> 
> Diffs
> -
> 
>   autotests/kconfigtest.cpp 9a2998647b5e5f54d63059172b727505a8ae1c80 
> 
> Diff: https://git.reviewboard.kde.org/r/126001/diff/
> 
> 
> Testing
> ---
> 
> Tested on both Qt 5.5.1 and Qt 5.6 (commit 
> e996d68f6130847637ba287518cff1289cfa48e5), tests all pass now.
> 
> 
> Thanks,
> 
> Matthew Dawson
> 
>

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

Review Request 126001: In KConfigTest::testEntryMap, convert QByteArray with nulls using a char *

[Matthew Dawson]

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

Review request for KDE Frameworks.


Repository: kconfig


Description
---

Due to https://codereview.qt-project.org/#/c/106473/, Qt 5.6 keeps null
characters in QByteArray -> QString conversions, which breaks this test as
one QByteArray contains nulls.  Instead, convert the QByteArray to const
char * first, so QString stops at the first null.

The actual  behaviour of KConfig is unchanged, as internally the conversion
always went through a const char *, which avoids creating QStrings with
null characters.


Diffs
-

  autotests/kconfigtest.cpp 9a2998647b5e5f54d63059172b727505a8ae1c80 

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


Testing
---

Tested on both Qt 5.5.1 and Qt 5.6 (commit 
e996d68f6130847637ba287518cff1289cfa48e5), tests all pass now.


Thanks,

Matthew Dawson

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

Re: Qt 5.6 behavior change with QByteArray(str, size) and QFile::encodeName()

[Matthew Dawson]

On November 7, 2015 08:07:11 PM David Faure wrote:
> KTar's unittests detected a behavior change in Qt.
> Thiago, is it intentional, or should it be fixed in Qt ?
> 
> KTar does
> 
> QString name = QFile::decodeName(QByteArray(buffer, 100));
> 
> where buffer is e.g. "filename\0\0\0\0\0\0[...]"
> 
> With Qt < 5.6 this would lead to name being equal to "filename".
> 
> With Qt 5.6 from git, it leads to name being equal to
> "filename\0\0\0\0\0etc.".
> 
> It appears that QUtfCodec doesn't stop at the first null byte anymore.
> 
> This appears to fix it:
> -name = QFile::decodeName(QByteArray(buffer, 100));
> +name = QFile::decodeName(QByteArray(buffer, qstrnlen(buffer,
> 100)));
> 
> but I have to find the other places in there, some more unittests still
> fail.
It appears this was a deliberate change, see 
https://codereview.qt-project.org/#/c/106473/ .  It affects KConfig as well, 
and I've posted a patch 
for its autotests.
-- 
Matthew

smime.p7s
Description: S/MIME cryptographic signature
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125598: Allow KConfig to use resources as fallback config files

[Matthew Dawson]

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


+1 from me as well.  For the prefix, I'd prefer to just use kconfig/ instead.  
When we get to KF6, having everyone move to kconfig6/ seems unnecessary and 
error prone for no gain.  Is there a reason kxmlgui used 5 in the name?

Otherwise, I don't think using kconfig(5)/ will be a problem, as it isn't 
likely someone will use that name in software named something else.

- Matthew Dawson


On Oct. 13, 2015, 2:20 a.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125598/
> ---
> 
> (Updated Oct. 13, 2015, 2:20 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Like KXMLGui, KConfig has now a fallback to config files bundled with the 
> application in resources.
> 
> Like kxmlgui, its in :/kconfig5.
> 
> This allows e.g. KTextEditor deployment without any installed files beside 
> the library itself and should easy the shipping of default configs for 
> non-linux/unix platforms.
> 
> Question is if :/kconfig5 is that nice a prefix, at least it is unique and 
> matches what we do with kxmlgui5.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 8213bc4 
>   autotests/fallbackconfigresources.qrc PRE-CREATION 
>   autotests/fallbackconfigresourcestest.cpp PRE-CREATION 
>   src/core/kconfig.cpp 7f03869 
> 
> Diff: https://git.reviewboard.kde.org/r/125598/diff/
> 
> 
> Testing
> ---
> 
> Added an unit test to show that the fallback resource is found and read.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

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

Re: ksycoca and kbuildsycoca, the next step

[Matthew Dawson]

On September 6, 2015 01:52:37 PM David Faure wrote:
> As a followup to recent discussions, I'm now looking into moving the
> recreating kbuildsycoca directly into the kservice library.
> 
> The rebuild now happens on demand when using any KService API and one the
> dirs with desktop files is more recent than the sycoca cache (that part is
> already in), so the next steps are:
Everything looks good from me, just one comment:

> 
> 1) kded no longer needs to watch the dirs with desktop files.
> I assume even the K menu doesn't just keep a cache of everything in memory,
> and use the KService/KServiceGroup API every time it's opened, so it should
> get the new stuff. However on demand means the old DBus signal
> databaseChanged() is either killed or at rebuild time by the app doing the
> rebuild (which could be later than when using file watching; possibly a
> very long time if no app calls into KService for a long time)...
Instead of killing the kded component completely, could we instead make it 
optional?  That way in a full KDE session, we monitor the directories as 
needed so we don't loose the file watching.  Then if an application using 
KService is launched outside the session, it falls back to this behaviour.

Though the problem from point 4 still exists.  Maybe make the signal be 
specific to the cache?  Since that would be more work then just a straight 
port that no one has time to write, I wouldn't care if the kded component is 
killed off currently, as long as it isn't a problem to bring back in a future 
version where it would be useful.
-- 
Matthew

smime.p7s
Description: S/MIME cryptographic signature
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: ksycoca and kbuildsycoca, the next step

[Matthew Dawson]

On September 7, 2015 12:03:59 AM you wrote:
> On Sunday 06 September 2015 17:59:13 Matthew Dawson wrote:
> > On September 6, 2015 01:52:37 PM David Faure wrote:
> > > As a followup to recent discussions, I'm now looking into moving the
> > > recreating kbuildsycoca directly into the kservice library.
> > > 
> > > The rebuild now happens on demand when using any KService API and one
> > > the
> > > dirs with desktop files is more recent than the sycoca cache (that part
> > > is
> > 
> > > already in), so the next steps are:
> > Everything looks good from me, just one comment:
> > > 1) kded no longer needs to watch the dirs with desktop files.
> > > I assume even the K menu doesn't just keep a cache of everything in
> > > memory,
> > > and use the KService/KServiceGroup API every time it's opened, so it
> > > should
> > > get the new stuff. However on demand means the old DBus signal
> > > databaseChanged() is either killed or at rebuild time by the app doing
> > > the
> > > rebuild (which could be later than when using file watching; possibly a
> > > very long time if no app calls into KService for a long time)...
> > 
> > Instead of killing the kded component completely, could we instead make it
> > optional?  That way in a full KDE session, we monitor the directories as
> > needed so we don't loose the file watching.  Then if an application using
> > KService is launched outside the session, it falls back to this behaviour.
> > 
> > Though the problem from point 4 still exists.  Maybe make the signal be
> > specific to the cache?  Since that would be more work then just a straight
> > port that no one has time to write, I wouldn't care if the kded component
> > is killed off currently, as long as it isn't a problem to bring back in a
> > future version where it would be useful.
> 
> I'm missing one point in your reasoning: what would we gain from keeping
> the kded code that watches desktop files and runs kbuildsycoca,
> if the apps do the same anyway, any time they need the sycoca contents?
I was thinking for things like the K menu, having a significant pause when 
opening the menu would be a bad UX.  And having each app implement its own 
caching layer would make things worse.

Also, since most people don't use make install to install/update programs, 
could we potentially miss something if we only watch the folder's mtime?

> 
> I guess this is an answer to my thoughts about the possible issue with
> nobody noticing a change for a long time, but I'm not sure the problem
> really exists.
> 
> Hmm. Best way to find out is to look at all users of the databaseChanged
> signal, I guess. I'll do that then.
> 
> I'd like to avoid solutions with "options" because it's more maintainance
> and more chances that the non-full-desktop case breaks (due to being less
> tested by most of us).
I agree completely.  And as long as we can bring back the option without too 
much extra work, I'm not worried.  I don't want to bikeshed this point, so 
whatever way you decide is fine by me.
-- 
Matthew

smime.p7s
Description: S/MIME cryptographic signature
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124138: Install kconfig_compiler into libexec

[Matthew Dawson]

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


+1, but again I'd like someone to check off the CMake foo as well.

- Matthew Dawson


On June 21, 2015, 12:15 a.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124138/
 ---
 
 (Updated June 21, 2015, 12:15 a.m.)
 
 
 Review request for KDE Frameworks, Matthew Dawson and Harald Sitter.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 This will make it end up in a platform-dependent prefix (i.e. /usr/lib64, 
 /usr/lib/arm-linux-gnueabihf, etc) rather than /usr/bin, making it possible 
 to have different kconfig_compiler versions installed, useful for 
 cross-compilation.
 
 
 Diffs
 -
 
   src/kconfig_compiler/CMakeLists.txt 0937f57 
 
 Diff: https://git.reviewboard.kde.org/r/124138/diff/
 
 
 Testing
 ---
 
 Changed and recompiled everything, nothing seems to break as cmake adapts the 
 *Target.cmake files to the new path.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


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

Re: Review Request 123872: Add TranslationDomain attribute to kconfig_compiler

[Matthew Dawson]

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

Ship it!


LGTM, thanks for adding the unit tests!  I have just one minor nitpick below 
(no need to repost the patch).  Also please be sure to include a Changelog: 
line in the commit message.


autotests/kconfig_compiler/test_translation.kcfg (line 4)
https://git.reviewboard.kde.org/r/123872/#comment55962

Nitpick: trailing space


- Matthew Dawson


On June 21, 2015, 4:47 p.m., Chusslove Illich wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123872/
 ---
 
 (Updated June 21, 2015, 4:47 p.m.)
 
 
 Review request for KDE Frameworks, Alexander Potashev and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 When using Ki18n as the translation system, library .kcfg files also need to 
 specify the translation domain. This is analogous to the TRANSLATION_DOMAIN 
 define in C++ code, and translationDomain attribute in .rc files.
 
 
 Diffs
 -
 
   autotests/kconfig_compiler/CMakeLists.txt f73aec0 
   autotests/kconfig_compiler/kconfigcompiler_test.cpp 77a31a3 
   autotests/kconfig_compiler/klocalizedstring.h e69de29 
   autotests/kconfig_compiler/test_translation.kcfg e69de29 
   autotests/kconfig_compiler/test_translation_kde.cpp.ref e69de29 
   autotests/kconfig_compiler/test_translation_kde.h.ref e69de29 
   autotests/kconfig_compiler/test_translation_kde.kcfgc e69de29 
   autotests/kconfig_compiler/test_translation_kde_domain.cpp.ref e69de29 
   autotests/kconfig_compiler/test_translation_kde_domain.h.ref e69de29 
   autotests/kconfig_compiler/test_translation_kde_domain.kcfgc e69de29 
   autotests/kconfig_compiler/test_translation_kde_domain_main.cpp e69de29 
   autotests/kconfig_compiler/test_translation_kde_main.cpp e69de29 
   autotests/kconfig_compiler/test_translation_qt.cpp.ref e69de29 
   autotests/kconfig_compiler/test_translation_qt.h.ref e69de29 
   autotests/kconfig_compiler/test_translation_qt.kcfgc e69de29 
   autotests/kconfig_compiler/test_translation_qt_main.cpp e69de29 
   src/kconfig_compiler/kconfig_compiler.cpp 7160bb5 
 
 Diff: https://git.reviewboard.kde.org/r/123872/diff/
 
 
 Testing
 ---
 
 Compiles.
 
 
 Thanks,
 
 Chusslove Illich
 


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

Re: Review Request 124129: Fix KAuthorized config file loading

[Matthew Dawson]

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


+1 from me, just one change below.  It's more semantically correct, and avoids 
a potential bug kicking around in KConfig.


src/core/kauthorized.cpp (line 181)
https://git.reviewboard.kde.org/r/124129/#comment55934

This should open with the NoGlobals flag, to avoid reading in the globals 
file twice.  This should be changed in all instances.


- Matthew Dawson


On June 19, 2015, 3:20 p.m., David Edmundson wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124129/
 ---
 
 (Updated June 19, 2015, 3:20 p.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 This was broken in porting, it used to be KGlobal::config() which opens
 the kdeglobals file, KSharedConfig::openConfig() opens a config file for
 that application, rather than a global file.
 
 
 Diffs
 -
 
   src/core/kauthorized.cpp 4280524dd292a464ad577b04ba1a03a52c7868f5 
 
 Diff: https://git.reviewboard.kde.org/r/124129/diff/
 
 
 Testing
 ---
 
 
 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 124138: Install kconfig_compiler into libexec

[Matthew Dawson]

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


+1 on moving it from me.  I don't think anyone will mind as it's a development 
tool.

However, please don't rename the executable.  It's nice that the executable 
already is namespaced for KF6 (may that be long in the future), and makes it 
easy to find if someone wasn't using CMake.

Also, please don't forget a Changelog: entry on the commit message.

Otherwise looks fine to me, but I'd prefer someone to check off the CMake part.

- Matthew Dawson


On June 20, 2015, 8:30 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124138/
 ---
 
 (Updated June 20, 2015, 8:30 p.m.)
 
 
 Review request for KDE Frameworks, Matthew Dawson and Harald Sitter.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 This will make it end up in a platform-dependent prefix (i.e. /usr/lib64, 
 /usr/lib/arm-linux-gnueabihf, etc) rather than /usr/bin, making it possible 
 to have different kconfig_compiler versions installed, useful for 
 cross-compilation.
 
 
 Diffs
 -
 
   src/kconfig_compiler/CMakeLists.txt 0937f57 
 
 Diff: https://git.reviewboard.kde.org/r/124138/diff/
 
 
 Testing
 ---
 
 Changed and recompiled everything, nothing seems to break as cmake adapts the 
 *Target.cmake files to the new path.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


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

Re: Review Request 124104: Make it possible to use kconfig_compiler from different sources

[Matthew Dawson]

 On June 15, 2015, 11:12 p.m., Matthew Dawson wrote:
  src/kconfig_compiler/CMakeLists.txt, line 16
  https://git.reviewboard.kde.org/r/124104/diff/1/?file=380232#file380232line16
 
  Does this mean a cross-compiled KConfig won't be built right for 
  development on the target?  If this happens regardless, is the cross 
  compiling still possible?
  
  My CMake foo is weak, so if no one knows an alternative, I'm fine.  I'd 
  just prefer KConfig to work on the target for development too.
 
 Aleix Pol Gonzalez wrote:
 Well, the idea is that if you're cross-compiling, the generated 
 executable won't work on the host architecture, by definition.
 
 In case it's needed, we could import an actual 
 KF5ConfigCompilerConfig.cmake from the host architecture. In this case it 
 doesn't really make sense though.

What I meant is that a cross-compilied KConfig would be built in such a way 
that transporting it to its native environment would allow it to be used as a 
complete development environment (assuming all the development related files 
were copied).  With this change, that doesn't work anymore.

It's a corner case, and if it breaks this otherwise fine RR, I'm not worried.  
But it would be nice to have.


- Matthew


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


On June 17, 2015, 9:19 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124104/
 ---
 
 (Updated June 17, 2015, 9:19 p.m.)
 
 
 Review request for Build System, KDE Frameworks, Matthew Dawson, and Harald 
 Sitter.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 Separates the tooling that is supposed to be used at run-time so that we can 
 specify which to use explicitly.
 
 Ideally this would be done automagically, for now we'll have to specify 
 -DKF5ConfigCompiler_DIR=...
 
 Additionally, we should make kconfig_compiler co-installable by putting it in 
 libexec, but I'd prefer to do that in a future patch. This same process 
 should also happen on other frameworks.
 
 
 Diffs
 -
 
   CMakeLists.txt f510e3e 
   KF5ConfigConfig.cmake.in b4e5f56 
   src/kconfig_compiler/CMakeLists.txt ec4a733 
 
 Diff: https://git.reviewboard.kde.org/r/124104/diff/
 
 
 Testing
 ---
 
 Works as expected on my debian chroot environment.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


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

Re: Review Request 124104: Make it possible to use kconfig_compiler from different sources

[Matthew Dawson]

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


+1 on the idea from me.  I don't know CMake well, so I'd prefer to have a CMake 
guru +1 this RR.  I do have a note below, but it isn't a blocker.


src/kconfig_compiler/CMakeLists.txt (line 16)
https://git.reviewboard.kde.org/r/124104/#comment55869

Does this mean a cross-compiled KConfig won't be built right for 
development on the target?  If this happens regardless, is the cross compiling 
still possible?

My CMake foo is weak, so if no one knows an alternative, I'm fine.  I'd 
just prefer KConfig to work on the target for development too.


- Matthew Dawson


On June 15, 2015, 1:59 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124104/
 ---
 
 (Updated June 15, 2015, 1:59 p.m.)
 
 
 Review request for Build System, KDE Frameworks, Matthew Dawson, and Harald 
 Sitter.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 Separates the tooling that is supposed to be used at run-time so that we can 
 specify which to use explicitly.
 
 Ideally this would be done automagically, for now we'll have to specify 
 -DKF5ConfigCompiler_DIR=...
 
 Additionally, we should make kconfig_compiler co-installable by putting it in 
 libexec, but I'd prefer to do that in a future patch. This same process 
 should also happen on other frameworks.
 
 
 Diffs
 -
 
   CMakeLists.txt f510e3e 
   KF5ConfigConfig.cmake.in b4e5f56 
   src/kconfig_compiler/CMakeLists.txt ec4a733 
 
 Diff: https://git.reviewboard.kde.org/r/124104/diff/
 
 
 Testing
 ---
 
 Works as expected on my debian chroot environment.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


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

Re: Review Request 123872: Add TranslationDomain attribute to kconfig_compiler

[Matthew Dawson]

 On May 21, 2015, 11:29 a.m., Matthew Dawson wrote:
  LGTM in general, but could you please add/update a unit test to test all 
  the possible cominbations of the translation?
 
 Chusslove Illich wrote:
 That would make the test depend on the Ki18n framework, which is not 
 acceptable for tier 1 frameworks.
 
 Alternatively, one could add fake klocalizedstring.h header, to shadow 
 any system header, and fill it with fake translation calls. But such test 
 would only check if generated translation call names are as expected, which 
 is trivially obvious from the code.

Even if it doesn't actually compile the code, I'd like to have something verify 
the calls are actually being put in the generated file.  It is trivial to see 
this RR is correct, but the problem is the future when someone (read: me) 
messes up the translation support.  I'd like to have something verify that 
block does what it should in the future, especially if that block gets a large 
refactor.


- Matthew


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


On May 21, 2015, 11:21 a.m., Chusslove Illich wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123872/
 ---
 
 (Updated May 21, 2015, 11:21 a.m.)
 
 
 Review request for KDE Frameworks, Alexander Potashev and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 When using Ki18n as the translation system, library .kcfg files also need to 
 specify the translation domain. This is analogous to the TRANSLATION_DOMAIN 
 define in C++ code, and translationDomain attribute in .rc files.
 
 
 Diffs
 -
 
   src/kconfig_compiler/kconfig_compiler.cpp 7160bb5 
 
 Diff: https://git.reviewboard.kde.org/r/123872/diff/
 
 
 Testing
 ---
 
 Compiles.
 
 
 Thanks,
 
 Chusslove Illich
 


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

Re: Review Request 123872: Add TranslationDomain attribute to kconfig_compiler

[Matthew Dawson]

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


LGTM in general, but could you please add/update a unit test to test all the 
possible cominbations of the translation?

- Matthew Dawson


On May 21, 2015, 11:21 a.m., Chusslove Illich wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123872/
 ---
 
 (Updated May 21, 2015, 11:21 a.m.)
 
 
 Review request for KDE Frameworks, Alexander Potashev and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 When using Ki18n as the translation system, library .kcfg files also need to 
 specify the translation domain. This is analogous to the TRANSLATION_DOMAIN 
 define in C++ code, and translationDomain attribute in .rc files.
 
 
 Diffs
 -
 
   src/kconfig_compiler/kconfig_compiler.cpp 7160bb5 
 
 Diff: https://git.reviewboard.kde.org/r/123872/diff/
 
 
 Testing
 ---
 
 Compiles.
 
 
 Thanks,
 
 Chusslove Illich
 


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

Re: Review Request 123367: Generate Q_PROPERTY entries out of KConfigSkeleton classes

[Matthew Dawson]

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

Ship it!


Ship It!

- Matthew Dawson


On April 25, 2015, 3:38 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123367/
 ---
 
 (Updated April 25, 2015, 3:38 p.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 The generation of those classes makes it useful to have these being used 
 within C++ application. This change makes it possible to use these classes 
 from QML as well.
 
 For each variable, exposes the getter. In case there's a setter, it will add 
 a notify signal and the setter to the property.
 
 
 Diffs
 -
 
   autotests/kconfig_compiler/CMakeLists.txt 0cca605 
   autotests/kconfig_compiler/kconfigcompiler_test.cpp 43623ce 
   autotests/kconfig_compiler/test13.cpp.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.h.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfg PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfgc PRE-CREATION 
   autotests/kconfig_compiler/test13main.cpp PRE-CREATION 
   autotests/kconfig_compiler/test_signal.cpp.ref 58e73ef 
   autotests/kconfig_compiler/test_signal.h.ref 19b8b40 
   src/kconfig_compiler/kconfig_compiler.cpp 5aae340 
 
 Diff: https://git.reviewboard.kde.org/r/123367/diff/
 
 
 Testing
 ---
 
 KConfig tests still pass.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


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

Re: Review Request 123367: Generate Q_PROPERTY entries out of KConfigSkeleton classes

[Matthew Dawson]

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



autotests/kconfig_compiler/test13.h.ref (line 55)
https://git.reviewboard.kde.org/r/123367/#comment54320

I mean to only avoid creating the brightnessChanged signal, since it isn't 
necessary to make this patch work.  brightnessModified, being required, is fine.

I'm only thinking of this as an optimization, and I'm not that worried 
about it anyways.

Also, I don't mind commiting the removal of that signal after this lands, 
as long as it happens before the release.  As long as that is true, no user 
code would be broken.  Doing it now is preferred :)


- Matthew Dawson


On April 24, 2015, 9:39 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123367/
 ---
 
 (Updated April 24, 2015, 9:39 p.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 The generation of those classes makes it useful to have these being used 
 within C++ application. This change makes it possible to use these classes 
 from QML as well.
 
 For each variable, exposes the getter. In case there's a setter, it will add 
 a notify signal and the setter to the property.
 
 
 Diffs
 -
 
   autotests/kconfig_compiler/CMakeLists.txt 0cca605 
   autotests/kconfig_compiler/kconfigcompiler_test.cpp 43623ce 
   autotests/kconfig_compiler/test13.cpp.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.h.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfg PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfgc PRE-CREATION 
   autotests/kconfig_compiler/test13main.cpp PRE-CREATION 
   autotests/kconfig_compiler/test_signal.cpp.ref 58e73ef 
   autotests/kconfig_compiler/test_signal.h.ref 19b8b40 
   src/kconfig_compiler/kconfig_compiler.cpp 5aae340 
 
 Diff: https://git.reviewboard.kde.org/r/123367/diff/
 
 
 Testing
 ---
 
 KConfig tests still pass.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


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

Re: Review Request 123367: Generate Q_PROPERTY entries out of KConfigSkeleton classes

[Matthew Dawson]

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



autotests/kconfig_compiler/test13.cpp.ref (line 18)
https://git.reviewboard.kde.org/r/123367/#comment54297

This needs to be wrapped with a KConfigCompilerSignallingItem so that 
itemChanged will be called.  Check line 41 in test_signal.cpp.ref for an 
example of how it's used.  kconfig_compiler has the code to emit it as 
necessary, it's just limited to signals at the moment.



autotests/kconfig_compiler/test_signal.cpp.ref (line 41)
https://git.reviewboard.kde.org/r/123367/#comment54296

Here is an example of KConfigCompilerSignallingItem in use.


- Matthew Dawson


On April 23, 2015, 8:53 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123367/
 ---
 
 (Updated April 23, 2015, 8:53 p.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 The generation of those classes makes it useful to have these being used 
 within C++ application. This change makes it possible to use these classes 
 from QML as well.
 
 For each variable, exposes the getter. In case there's a setter, it will add 
 a notify signal and the setter to the property.
 
 
 Diffs
 -
 
   autotests/kconfig_compiler/CMakeLists.txt 0cca605 
   autotests/kconfig_compiler/kconfigcompiler_test.cpp 43623ce 
   autotests/kconfig_compiler/test13.cpp.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.h.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfg PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfgc PRE-CREATION 
   autotests/kconfig_compiler/test13main.cpp PRE-CREATION 
   autotests/kconfig_compiler/test_signal.cpp.ref 58e73ef 
   autotests/kconfig_compiler/test_signal.h.ref 19b8b40 
   src/kconfig_compiler/kconfig_compiler.cpp 5aae340 
 
 Diff: https://git.reviewboard.kde.org/r/123367/diff/
 
 
 Testing
 ---
 
 KConfig tests still pass.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


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

Re: Review Request 123367: Generate Q_PROPERTY entries out of KConfigSkeleton classes

[Matthew Dawson]

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


Ok, I think this should be last set of comments.  One thing I missed, and one 
last thought I had looking at what the final diff looks like.


autotests/kconfig_compiler/test13.h.ref (line 36)
https://git.reviewboard.kde.org/r/123367/#comment54313

Sorry, I missed this change.  This also needs to emit brightnessModified 
(while also change mSettingChanged if brightnessChanged is created).



autotests/kconfig_compiler/test13.h.ref (line 55)
https://git.reviewboard.kde.org/r/123367/#comment54314

Also, I guess brightnessChanged and usrWriteconfig don't need to be 
generated anymore if a signal isn't being produced.

Considering how much time this has taken, I'd be ok leaving the 
brightnessChanged signal alone for this review, and removing it later.


- Matthew Dawson


On April 24, 2015, 9:39 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123367/
 ---
 
 (Updated April 24, 2015, 9:39 p.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 The generation of those classes makes it useful to have these being used 
 within C++ application. This change makes it possible to use these classes 
 from QML as well.
 
 For each variable, exposes the getter. In case there's a setter, it will add 
 a notify signal and the setter to the property.
 
 
 Diffs
 -
 
   autotests/kconfig_compiler/CMakeLists.txt 0cca605 
   autotests/kconfig_compiler/kconfigcompiler_test.cpp 43623ce 
   autotests/kconfig_compiler/test13.cpp.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.h.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfg PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfgc PRE-CREATION 
   autotests/kconfig_compiler/test13main.cpp PRE-CREATION 
   autotests/kconfig_compiler/test_signal.cpp.ref 58e73ef 
   autotests/kconfig_compiler/test_signal.h.ref 19b8b40 
   src/kconfig_compiler/kconfig_compiler.cpp 5aae340 
 
 Diff: https://git.reviewboard.kde.org/r/123367/diff/
 
 
 Testing
 ---
 
 KConfig tests still pass.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


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

Re: Review Request 123367: Generate Q_PROPERTY entries out of KConfigSkeleton classes

[Matthew Dawson]

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


Looks good, one last comment:


autotests/kconfig_compiler/test13.cpp.ref (line 33)
https://git.reviewboard.kde.org/r/123367/#comment54309

Is there a reason this got moved back?


- Matthew Dawson


On April 24, 2015, 12:56 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123367/
 ---
 
 (Updated April 24, 2015, 12:56 p.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 The generation of those classes makes it useful to have these being used 
 within C++ application. This change makes it possible to use these classes 
 from QML as well.
 
 For each variable, exposes the getter. In case there's a setter, it will add 
 a notify signal and the setter to the property.
 
 
 Diffs
 -
 
   autotests/kconfig_compiler/CMakeLists.txt 0cca605 
   autotests/kconfig_compiler/kconfigcompiler_test.cpp 43623ce 
   autotests/kconfig_compiler/test13.cpp.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.h.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfg PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfgc PRE-CREATION 
   autotests/kconfig_compiler/test13main.cpp PRE-CREATION 
   autotests/kconfig_compiler/test_signal.cpp.ref 58e73ef 
   autotests/kconfig_compiler/test_signal.h.ref 19b8b40 
   src/kconfig_compiler/kconfig_compiler.cpp 5aae340 
 
 Diff: https://git.reviewboard.kde.org/r/123367/diff/
 
 
 Testing
 ---
 
 KConfig tests still pass.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


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

Re: Review Request 123479: Adapt to Qt 5.6 changes and prevent nullbytes in QStrings.

[Matthew Dawson]

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


LGTM, but I think having/modifying a unit test for this change would be useful.

- Matthew Dawson


On April 23, 2015, 12:50 p.m., Milian Wolff wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123479/
 ---
 
 (Updated April 23, 2015, 12:50 p.m.)
 
 
 Review request for KDE Frameworks, David Faure, Marc Mutz, and Volker Krause.
 
 
 Repository: kcoreaddons
 
 
 Description
 ---
 
 The len in inotify_event includes nulls of the name. To prevent
 them from being included in the QString/QByteArray we must filter
 them manually with a recent Qt 5 dev build now. See also:
 
 https://codereview.qt-project.org/#/c/106473/
 
 REVIEW: 123479
 
 
 Diffs
 -
 
   src/lib/io/kdirwatch.cpp a75fae012c59021996b46845db2e97abb5526930 
 
 Diff: https://git.reviewboard.kde.org/r/123479/diff/
 
 
 Testing
 ---
 
 I used the test and looked at the output and also ran it against a patched 
 qtbase with this:
 
 https://paste.kde.org/pmoue241d
 
 
 Thanks,
 
 Milian Wolff
 


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

Re: Review Request 123479: Adapt to Qt 5.6 changes and prevent nullbytes in QStrings.

[Matthew Dawson]

 On April 23, 2015, 12:55 p.m., Milian Wolff wrote:
  How are kf5 releases handled btw? Do I need to push this into some branch? 
  There are none in kcoreaddons. This patch here should probably be put into 
  a patch release (if we do this for kf5).
 
 Luigi Toscano wrote:
 The current schedule is one release every month, no patch releases, 
 master always build-able.

KF5 has monthly releases from the master branch, and no patch release unless 
something critical shows up.  So this should just get pushed to master once the 
ship it is given.


- Matthew


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


On April 23, 2015, 12:50 p.m., Milian Wolff wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123479/
 ---
 
 (Updated April 23, 2015, 12:50 p.m.)
 
 
 Review request for KDE Frameworks, David Faure, Marc Mutz, and Volker Krause.
 
 
 Repository: kcoreaddons
 
 
 Description
 ---
 
 The len in inotify_event includes nulls of the name. To prevent
 them from being included in the QString/QByteArray we must filter
 them manually with a recent Qt 5 dev build now. See also:
 
 https://codereview.qt-project.org/#/c/106473/
 
 REVIEW: 123479
 
 
 Diffs
 -
 
   src/lib/io/kdirwatch.cpp a75fae012c59021996b46845db2e97abb5526930 
 
 Diff: https://git.reviewboard.kde.org/r/123479/diff/
 
 
 Testing
 ---
 
 I used the test and looked at the output and also ran it against a patched 
 qtbase with this:
 
 https://paste.kde.org/pmoue241d
 
 
 Thanks,
 
 Milian Wolff
 


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

Re: Review Request 123479: Adapt to Qt 5.6 changes and prevent nullbytes in QStrings.

[Matthew Dawson]

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


+1 from me, but I don't want to hit ship it without hearing from someone else.

- Matthew Dawson


On April 23, 2015, 1:19 p.m., Milian Wolff wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123479/
 ---
 
 (Updated April 23, 2015, 1:19 p.m.)
 
 
 Review request for KDE Frameworks, David Faure, Marc Mutz, and Volker Krause.
 
 
 Repository: kcoreaddons
 
 
 Description
 ---
 
 The len in inotify_event includes nulls of the name. To prevent
 them from being included in the QString/QByteArray we must filter
 them manually with a recent Qt 5 dev build now. See also:
 
 https://codereview.qt-project.org/#/c/106473/
 
 REVIEW: 123479
 
 
 Diffs
 -
 
   autotests/kdirwatch_unittest.cpp 83b5b410e987fb45a08f8251ec65496c8074b077 
   src/lib/io/kdirwatch.cpp a75fae012c59021996b46845db2e97abb5526930 
 
 Diff: https://git.reviewboard.kde.org/r/123479/diff/
 
 
 Testing
 ---
 
 I used the test and looked at the output and also ran it against a patched 
 qtbase with this:
 
 https://paste.kde.org/pmoue241d
 
 
 Thanks,
 
 Milian Wolff
 


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

Re: Review Request 123367: Generate Q_PROPERTY entries out of KConfigSkeleton classes

[Matthew Dawson]

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



autotests/kconfig_compiler/test13.cpp.ref (line 18)
https://git.reviewboard.kde.org/r/123367/#comment54237

This doesn't actually use the notifyFunction from above, so any external 
changes are not seen when the class is reloaded.

Also, reusing the notify function in this manner will only cause the signal 
to trigger when the class is saved, AFAICS.  I think just moving the emission 
of brightnessModified to itemChanged would work, along with an equality check.


Thanks for bearing with me on all these changes :)

- Matthew Dawson


On April 22, 2015, 9:20 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123367/
 ---
 
 (Updated April 22, 2015, 9:20 p.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 The generation of those classes makes it useful to have these being used 
 within C++ application. This change makes it possible to use these classes 
 from QML as well.
 
 For each variable, exposes the getter. In case there's a setter, it will add 
 a notify signal and the setter to the property.
 
 
 Diffs
 -
 
   autotests/kconfig_compiler/CMakeLists.txt 0cca605 
   autotests/kconfig_compiler/kconfigcompiler_test.cpp 43623ce 
   autotests/kconfig_compiler/test13.cpp.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.h.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfg PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfgc PRE-CREATION 
   autotests/kconfig_compiler/test13main.cpp PRE-CREATION 
   autotests/kconfig_compiler/test_signal.cpp.ref 58e73ef 
   autotests/kconfig_compiler/test_signal.h.ref 19b8b40 
   src/kconfig_compiler/kconfig_compiler.cpp 5aae340 
 
 Diff: https://git.reviewboard.kde.org/r/123367/diff/
 
 
 Testing
 ---
 
 KConfig tests still pass.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


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

Re: Review Request 123367: Generate Q_PROPERTY entries out of KConfigSkeleton classes

[Matthew Dawson]

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



autotests/kconfig_compiler/test13.h.ref (line 38)
https://git.reviewboard.kde.org/r/123367/#comment54192

I'm not sure if brightnessChanged is the right signal to use here, as 
calling setBrightness won't trigger it.  I'm not sure if users would expect it 
to change then, or when this object actually gets saved.

Thoughts?



src/kconfig_compiler/kconfig_compiler.cpp (line 100)
https://git.reviewboard.kde.org/r/123367/#comment54191

Is there a reason not to generate Q_PROPERTIES for all classes, or at least 
do it by default?  This seems like a useful thing to have every class use, so 
adding another configuration only reduces its visibility.


- Matthew Dawson


On April 22, 2015, 10:51 a.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123367/
 ---
 
 (Updated April 22, 2015, 10:51 a.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 The generation of those classes makes it useful to have these being used 
 within C++ application. This change makes it possible to use these classes 
 from QML as well.
 
 For each variable, exposes the getter. In case there's a setter, it will add 
 a notify signal and the setter to the property.
 
 
 Diffs
 -
 
   autotests/kconfig_compiler/CMakeLists.txt 0cca605 
   autotests/kconfig_compiler/kconfigcompiler_test.cpp 43623ce 
   autotests/kconfig_compiler/test13.cpp.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.h.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfg PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfgc PRE-CREATION 
   autotests/kconfig_compiler/test13main.cpp PRE-CREATION 
   autotests/kconfig_compiler/test_signal.cpp.ref 58e73ef 
   autotests/kconfig_compiler/test_signal.h.ref 19b8b40 
   src/kconfig_compiler/kconfig_compiler.cpp 5aae340 
 
 Diff: https://git.reviewboard.kde.org/r/123367/diff/
 
 
 Testing
 ---
 
 KConfig tests still pass.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


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

Re: Review Request 123367: Generate Q_PROPERTY entries out of KConfigSkeleton classes

[Matthew Dawson]

 On April 22, 2015, 10:53 a.m., Matthew Dawson wrote:
  src/kconfig_compiler/kconfig_compiler.cpp, line 100
  https://git.reviewboard.kde.org/r/123367/diff/3/?file=361168#file361168line100
 
  Is there a reason not to generate Q_PROPERTIES for all classes, or at 
  least do it by default?  This seems like a useful thing to have every class 
  use, so adding another configuration only reduces its visibility.
 
 Aleix Pol Gonzalez wrote:
 To minimize changes. I agree it would be interesting, I can do it if you 
 (collectively) are on board.
 
 Do you have an idea of what should the setting be called instead? 
 DoNotGenerateProperties?

I'd just change the default to be true.  Based upon what I remember about UX, 
double negatives should be avoided.


- Matthew


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


On April 22, 2015, 10:51 a.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123367/
 ---
 
 (Updated April 22, 2015, 10:51 a.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 The generation of those classes makes it useful to have these being used 
 within C++ application. This change makes it possible to use these classes 
 from QML as well.
 
 For each variable, exposes the getter. In case there's a setter, it will add 
 a notify signal and the setter to the property.
 
 
 Diffs
 -
 
   autotests/kconfig_compiler/CMakeLists.txt 0cca605 
   autotests/kconfig_compiler/kconfigcompiler_test.cpp 43623ce 
   autotests/kconfig_compiler/test13.cpp.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.h.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfg PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfgc PRE-CREATION 
   autotests/kconfig_compiler/test13main.cpp PRE-CREATION 
   autotests/kconfig_compiler/test_signal.cpp.ref 58e73ef 
   autotests/kconfig_compiler/test_signal.h.ref 19b8b40 
   src/kconfig_compiler/kconfig_compiler.cpp 5aae340 
 
 Diff: https://git.reviewboard.kde.org/r/123367/diff/
 
 
 Testing
 ---
 
 KConfig tests still pass.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


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

Re: Review Request 123367: Generate Q_PROPERTY entries out of KConfigSkeleton classes

[Matthew Dawson]

 On April 22, 2015, 10:53 a.m., Matthew Dawson wrote:
  src/kconfig_compiler/kconfig_compiler.cpp, line 100
  https://git.reviewboard.kde.org/r/123367/diff/3/?file=361168#file361168line100
 
  Is there a reason not to generate Q_PROPERTIES for all classes, or at 
  least do it by default?  This seems like a useful thing to have every class 
  use, so adding another configuration only reduces its visibility.
 
 Aleix Pol Gonzalez wrote:
 To minimize changes. I agree it would be interesting, I can do it if you 
 (collectively) are on board.
 
 Do you have an idea of what should the setting be called instead? 
 DoNotGenerateProperties?
 
 Matthew Dawson wrote:
 I'd just change the default to be true.  Based upon what I remember about 
 UX, double negatives should be avoided.
 
 Aleix Pol Gonzalez wrote:
 Ok, let's do it in a separate iteration of the patch though. This is 
 going to make a huge patch as it will require changing most of the unit tests.

Ok, sounds good.  For that patch, it would be good to leave some unit tests not 
generating properties, to ensure the negative works as well.


- Matthew


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


On April 22, 2015, 10:51 a.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123367/
 ---
 
 (Updated April 22, 2015, 10:51 a.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 The generation of those classes makes it useful to have these being used 
 within C++ application. This change makes it possible to use these classes 
 from QML as well.
 
 For each variable, exposes the getter. In case there's a setter, it will add 
 a notify signal and the setter to the property.
 
 
 Diffs
 -
 
   autotests/kconfig_compiler/CMakeLists.txt 0cca605 
   autotests/kconfig_compiler/kconfigcompiler_test.cpp 43623ce 
   autotests/kconfig_compiler/test13.cpp.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.h.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfg PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfgc PRE-CREATION 
   autotests/kconfig_compiler/test13main.cpp PRE-CREATION 
   autotests/kconfig_compiler/test_signal.cpp.ref 58e73ef 
   autotests/kconfig_compiler/test_signal.h.ref 19b8b40 
   src/kconfig_compiler/kconfig_compiler.cpp 5aae340 
 
 Diff: https://git.reviewboard.kde.org/r/123367/diff/
 
 
 Testing
 ---
 
 KConfig tests still pass.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


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

Re: Review Request 123367: Generate Q_PROPERTY entries out of KConfigSkeleton classes

[Matthew Dawson]

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


If the first issue doesn't fit into this commit, this has a ship it from me 
after the two nitpicks are fixed.


autotests/kconfig_compiler/test13.h.ref (line 51)
https://git.reviewboard.kde.org/r/123367/#comment54201

This looks better, but I just thought of one other case: when the 
underlying KConfig class gets updated and the KConfigSkeleton updates.  If that 
isn't too much work to do here, I'd appreciate it if it's done now.  Otherwise 
please file a bug against KConfig so I don't forget.



src/kconfig_compiler/kconfig_compiler.cpp (line 1418)
https://git.reviewboard.kde.org/r/123367/#comment54202

Nitpick: Please always include braces on if statements.



src/kconfig_compiler/kconfig_compiler.cpp (line 1452)
https://git.reviewboard.kde.org/r/123367/#comment54203

Nitpick: braces here too.


- Matthew Dawson


On April 22, 2015, 12:07 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123367/
 ---
 
 (Updated April 22, 2015, 12:07 p.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 The generation of those classes makes it useful to have these being used 
 within C++ application. This change makes it possible to use these classes 
 from QML as well.
 
 For each variable, exposes the getter. In case there's a setter, it will add 
 a notify signal and the setter to the property.
 
 
 Diffs
 -
 
   autotests/kconfig_compiler/CMakeLists.txt 0cca605 
   autotests/kconfig_compiler/kconfigcompiler_test.cpp 43623ce 
   autotests/kconfig_compiler/test13.cpp.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.h.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfg PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfgc PRE-CREATION 
   autotests/kconfig_compiler/test13main.cpp PRE-CREATION 
   autotests/kconfig_compiler/test_signal.cpp.ref 58e73ef 
   autotests/kconfig_compiler/test_signal.h.ref 19b8b40 
   src/kconfig_compiler/kconfig_compiler.cpp 5aae340 
 
 Diff: https://git.reviewboard.kde.org/r/123367/diff/
 
 
 Testing
 ---
 
 KConfig tests still pass.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


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

Re: Review Request 123367: Generate Q_PROPERTY entries out of KConfigSkeleton classes

[Matthew Dawson]

 On April 22, 2015, 5 p.m., Albert Astals Cid wrote:
  autotests/kconfig_compiler/test13.h.ref, line 20
  https://git.reviewboard.kde.org/r/123367/diff/5/?file=362514#file362514line20
 
  Why is there no brightness property anymore?

I still see it on line 40.


- Matthew


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


On April 22, 2015, 12:07 p.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123367/
 ---
 
 (Updated April 22, 2015, 12:07 p.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 The generation of those classes makes it useful to have these being used 
 within C++ application. This change makes it possible to use these classes 
 from QML as well.
 
 For each variable, exposes the getter. In case there's a setter, it will add 
 a notify signal and the setter to the property.
 
 
 Diffs
 -
 
   autotests/kconfig_compiler/CMakeLists.txt 0cca605 
   autotests/kconfig_compiler/kconfigcompiler_test.cpp 43623ce 
   autotests/kconfig_compiler/test13.cpp.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.h.ref PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfg PRE-CREATION 
   autotests/kconfig_compiler/test13.kcfgc PRE-CREATION 
   autotests/kconfig_compiler/test13main.cpp PRE-CREATION 
   autotests/kconfig_compiler/test_signal.cpp.ref 58e73ef 
   autotests/kconfig_compiler/test_signal.h.ref 19b8b40 
   src/kconfig_compiler/kconfig_compiler.cpp 5aae340 
 
 Diff: https://git.reviewboard.kde.org/r/123367/diff/
 
 
 Testing
 ---
 
 KConfig tests still pass.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


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

Re: [Kde-games-devel] Data migration issue

[Matthew Dawson]

On March 24, 2015 12:59:26 AM Albert Astals Cid wrote:
 El Dilluns, 23 de març de 2015, a les 00:25:14, Matthew Dawson va escriure:
  On March 22, 2015 06:19:46 PM David Faure wrote:
   Any input?
  
  I'm generally fine with this implementation, it seems like it would fix
  the
  problem.
  
  I do have an alternate: would it be better to try and call into KConfig
  from KCoreAddons, iff both are loaded into the same process so that all
  KSharedConfigs could be reloaded after migration?  It creates a hidden
  dependency between them, but as long as it doesn't error out over while
  making the call, it shouldn't matter.  It would also handle the case the
  application isn't using frameworksintegration, but opened a KSharedConfig
  before.
 
 But then that's the application fault and it shouldn't be doing that thing
 before migrating their configs.
 
 The issue here is that frameworksintegration is there messing with the
 configs before the app can decide to migrate them.
I agree.  This just prevents anything from messing it up, which is nice to 
have.  I can flesh it out if this approach seems reasonable.

If everyone else feels David's patches are the best way forward, I'm fine with 
that as well.  This was just an alternate idea, but it may be more complicated 
then we really want to entertain anyways :)
-- 
Matthew

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: [Kde-games-devel] Data migration issue

[Matthew Dawson]

On March 22, 2015 06:19:46 PM David Faure wrote:
 On Saturday 21 March 2015 23:53:07 Albert Astals Cid wrote:
  Too late for KDE Applications 15.04 though now that i think, so maybe we
  actually have to suggest everybody using the migrator to add those extra
  lines?
 
 OK, that means:
 
 --- a/src/lib/util/kdelibs4configmigrator.h
 +++ b/src/lib/util/kdelibs4configmigrator.h
 @@ -60,6 +60,10 @@ public:
   *look for old data to migrate as well
   * }
   * @endcode
 + *
 + * IMPORTANT: after migrate() returns true, call
 KSharedConfig::openConfig()-reparseConfiguration() + * so that the
 global config (very likely already created by e.g. the platform integration
 plugin) + * can see the migrated data.
   */
  bool migrate();
 
 But wait, this makes me realize another solution: kdelibs4configmigrator
 could load the framework integration plugin, which would take care of this,
 if installed.
 
 Patch attached. Tested, at least to the extent that the slot is indeed being
 called.
 
 Obviously that's still too late for KDE Applications 15.04, so maybe fix
 the apps that you want to see fixed for 15.04 with a
 #if KCOREADDONS_VERSION  0x050900
 KSharedConfig::openConfig()-reparseConfiguration();
 #endif
 
 and KF 5.9 will make this unnecessary, i.e. the doc change won't get
 committed.
 
 Any input?
I'm generally fine with this implementation, it seems like it would fix the 
problem.

I do have an alternate: would it be better to try and call into KConfig from 
KCoreAddons, iff both are loaded into the same process so that all 
KSharedConfigs could be reloaded after migration?  It creates a hidden 
dependency between them, but as long as it doesn't error out over while making 
the call, it shouldn't matter.  It would also handle the case the application 
isn't using frameworksintegration, but opened a KSharedConfig before.

My idea for calling across the libraries looks something like what I pushed 
here: g...@git.kde.org:scratch/mdawson/kcoreaddons_talk_to_kconfig_base (hasn't 
propagated to the anon side yet).  Consider liblib1.so to be KConfig, and 
liblib2.so to be KCoreAddons.  

dlsym isn't portable though, so this idea would need some work on Windows.  
I'm just not sure how to implement that simply without OS specific code.  But 
I'm happy either way :)
-- 
Matthew

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: [Kde-games-devel] Data migration issue

[Matthew Dawson]

On March 21, 2015 04:00:11 PM Mathias Kraus wrote:
 ===
 KConfig config(QLatin1String(kminesrc));
 config.reparseConfiguration();
 ===
Sorry, my suggestion of using reparseConfiguration wasn't clear.  I meant 
using it against the global KSharedConfig, not a newly created KConfig.  
Instead, try:

KSharedConfig::openConfig()-reparseConfiguration();

And see if that helps (untested here).
-- 
Matthew

smime.p7s
Description: S/MIME cryptographic signature
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: [Kde-games-devel] Data migration issue

[Matthew Dawson]

On March 21, 2015 04:29:25 PM Mathias Kraus wrote:
 Am Samstag, 21. März 2015, 11:06:02 schrieb Matthew Dawson:
  On March 21, 2015 04:00:11 PM Mathias Kraus wrote:
   ===
   KConfig config(QLatin1String(kminesrc));
   config.reparseConfiguration();
   ===
  
  Sorry, my suggestion of using reparseConfiguration wasn't clear.  I meant
  using it against the global KSharedConfig, not a newly created KConfig.
  Instead, try:
  
  KSharedConfig::openConfig()-reparseConfiguration();
  
  And see if that helps (untested here).
 
 Yes, that works. Should have told you what I tried.
Excellent!  So now we at least know what the problem is.

 Thanks very much. Now we need to update all kf5 games. I will do it for
 granatier but don't know if I have enough time to do it for all games. Will
 try to do it though.
 
 Could this also be necessary for non-game applications?
I don't think this can be solved universally, as KConfig and KCoreAddons can't 
depend upon each other.  My only thought would be to have KCoreAddons lookup a 
symbol at runtime, and call that to reload all cached KSharedConfigs, but I 
have no idea if that is feasible, never mind if it will work.  Failing that, 
we should update the documentation to mention the need to reload any 
KSharedConfigs after migration.

Thoughts?
-- 
Matthew

smime.p7s
Description: S/MIME cryptographic signature
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: [Kde-games-devel] Data migration issue

[Matthew Dawson]

On March 3, 2015 10:59:21 PM Mathias Kraus wrote:
 Hi,
 
 I noticed a problem with the data migration in the kdegames transition from
 kdelibs4 to kf5 and Albert Astals Cid suggested to ask frameworks devel for
 advice.
 
 In short, when the migration is run after the QCoreApplication is created,
 the migrated config is not used but the default values. This results in a
 lost migration if the users changes some settings during the first run of
 the kf5 version of the game. Also, if the user plays a game and wins, all
 the previous high scores will be lost. Ian Wadham assumes there will be
 also other issues.
 
 If the migration is run before the creation of the QCoreApplication, it
 seems to work correct.
Taking a look into what is happening, it appears that other libraries are 
accessing the kminesrc file once QCoreApplication is created by using 
KSharedConfig::openConfig.  This function caches that instance of 
KSharedConfig, which is working ontop of an empty file and thus does nothing.

One backtrace I got looks like:
#5  0x75adc3c0 in KSharedConfig::openConfig (_fileName=..., flags=..., 
resType=QStandardPaths::GenericConfigLocation) at 
/home/matthew/kde/kconfig/src/core/ksharedconfig.cpp:87
#6  0x7fffe21ed296 in KHintsSettings::KHintsSettings (this=0x683380) at 
/usr/src/debug/kde-
frameworks/frameworkintegration-/frameworkintegration-/src/platformtheme/khintssettings.cpp:65
#7  0x7fffe21e94f7 in KdePlatformTheme::loadSettings (this=0x67f930) at 
/usr/src/debug/kde-
frameworks/frameworkintegration-/frameworkintegration-/src/platformtheme/kdeplatformtheme.cpp:121
#8  0x7fffe21fc6bb in KdePlatformThemePlugin::create (this=optimized 
out, key=..., paramList=...) at /usr/src/debug/kde-
frameworks/frameworkintegration-/frameworkintegration-/src/platformtheme/main.cpp:52
#9  0x74aaa44e in qLoadPlugin1QPlatformTheme, QPlatformThemePlugin, 
QStringList (parameter1=..., key=..., loader=0x74f63ab0 
_ZZN12_GLOBAL__N_112Q_QGS_loader13innerFunctionEvE6holder) at 
/usr/include/qt5/QtCore/5.4.0/QtCore/private/qfactoryloader_p.h:107
#10 QPlatformThemeFactory::create (key=..., platformPluginPath=...) at 
kernel/qplatformthemefactory.cpp:64
#11 0x74ab4012 in init_platform (argv=0x7fffce58, 
argc=@0x7fffcc7c: 1, platformThemeName=..., platformPluginPath=..., 
pluginArgument=...) at kernel/qguiapplication.cpp:1045
#12 QGuiApplicationPrivate::createPlatformIntegration (this=0x63dec0) at 
kernel/qguiapplication.cpp:1166
#13 0x74ab4ced in QGuiApplicationPrivate::createEventDispatcher 
(this=optimized out) at kernel/qguiapplication.cpp:1183
#14 0x74704126 in QCoreApplication::init 
(this=this@entry=0x7fffcd30) at kernel/qcoreapplication.cpp:727
#15 0x74704186 in QCoreApplication::QCoreApplication 
(this=0x7fffcd30, p=...) at kernel/qcoreapplication.cpp:650
#16 0x74ab6ea9 in QGuiApplication::QGuiApplication 
(this=0x7fffcd30, p=...) at kernel/qguiapplication.cpp:560
#17 0x750c0c05 in QApplication::QApplication (this=0x7fffcd30, 
argc=@0x7fffcc7c: 1, argv=0x7fffce58, _internal=328704) at 
kernel/qapplication.cpp:562
#18 0x0042d68c in main (argc=1, argv=0x7fffce58) at 
/home/matthew/kde/kmines/main.cpp:35

Which I think might be avoidable anyways, as the hints should only read from 
the globals?  Migrating first would of course avoid this issue (but see 
below), and I think calling reparseConfiguration after migrating may also 
work.  Alternately, tracking down why kminesrc is opened early might be 
another solution.

 
 Laurent Montel, the creator of kdelibs4configmigrator, suggests the former
 procedure (first QCoreApplication, then migration), though.
If kdelibs4configmigrator uses KConfig itself for anything, then it really 
should only be used after QCoreApplication is created.  Doing otherwise is 
very fragile and may break in the future.

Hope that helps,
-- 
Matthew

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: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.

[Matthew Dawson]

 On Feb. 21, 2015, 3:19 p.m., Matthew Dawson wrote:
  Everything looked good, but when I tried to rerun the tests to make sure 
  everything is ok the kconfig_in_global_object test I'm getting an abort 
  with the same error message.  Backtrace:
  ```
  #5  0x7791b630 in qAppName () at kernel/qcoreapplication.cpp:541
  #6  0x778a9c76 in QLockFilePrivate::tryLock_sys 
  (this=this@entry=0x608ec0) at io/qlockfile_unix.cpp:140
  #7  0x7783c1ec in QLockFile::tryLock (this=optimized out, 
  timeout=-1) at io/qlockfile.cpp:208
  #8  0x77ba17db in KConfigIniBackend::lock (this=0x608fe0) at 
  /home/matthew/kde/kconfig/src/core/kconfigini.cpp:634
  #9  0x77b84ce0 in KConfigPrivate::lockLocal (this=0x608b40) at 
  /home/matthew/kde/kconfig/src/core/kconfig.cpp:104
  #10 0x77b86aa9 in KConfig::sync (this=0x606ae0) at 
  /home/matthew/kde/kconfig/src/core/kconfig.cpp:415
  #11 0x77b85c28 in KConfig::~KConfig (this=0x606ae0, 
  __in_chrg=optimized out) at 
  /home/matthew/kde/kconfig/src/core/kconfig.cpp:267
  #12 0x77b85c82 in KConfig::~KConfig (this=0x606ae0, 
  __in_chrg=optimized out) at 
  /home/matthew/kde/kconfig/src/core/kconfig.cpp:270
  #13 0x00401844 in Tester::~Tester (this=0x6030e8 (anonymous 
  namespace)::Q_QGS_globalTestObject::innerFunction()::holder, 
  __in_chrg=optimized out)
  at /home/matthew/kde/kconfig/autotests/kconfig_in_global_object.cpp:56
  ```
  Which seems to be second issue we found.  I'm not sure how to solve this 
  issue yet, but I'd be happy to just document the issue and strike this test.
  
  I only get this when the configuration file does not exist before the 
  program executes, which may be why we missed it from the earlier patches.
 
 David Faure wrote:
 Indeed. Good catch.
 
 Fixed in Qt. https://codereview.qt-project.org/106953.
 
 David Faure wrote:
 Ah, and https://codereview.qt-project.org/106954

Ok, sounds good :).  I think it would be good to fix as much of the issue this 
RR covers while we wait for that patch to ship in a released Qt version (since 
I assume its Qt 5.5 or 5.6 territory).

So, I think the ksharedconfig_in_global_object test and and the changes in the 
src/ folder should be committed now, so that KSharedConfig creation is safe.  
After the Qt patch lands, this RR (or a new one) can be updated to test 
everything, given a recent enough copy of Qt (basically kconfig_in_global 
object).  Thoughts?  If you are happy with this plan, the first part has a ship 
it from me.


- Matthew


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


On Feb. 21, 2015, 2:58 p.m., David Faure wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122232/
 ---
 
 (Updated Feb. 21, 2015, 2:58 p.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 kconfig_in_global_object.cpp comes from kdelibs4support
 (after porting to Q_GLOBAL_STATIC)
 
 ksharedconfig_in_global_object.cpp is now in kdelibs4 too
 (where it works) and reproduces Albert's KgDifficulty testcase.
 
 
 Diffs
 -
 
   autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace 
   autotests/kconfig_in_global_object.cpp PRE-CREATION 
   autotests/ksharedconfig_in_global_object.cpp PRE-CREATION 
   src/core/kconfig.cpp 1da816faea5548d2f529755d6538a142a2193720 
   src/core/ksharedconfig.h d0791e461804d421010bc5f82873371a4d3bc992 
 
 Diff: https://git.reviewboard.kde.org/r/122232/diff/
 
 
 Testing
 ---
 
 Both tests pass and the QCoreApplication::arguments warning (because called 
 after qApp is destroyed) is gone.
 
 
 Thanks,
 
 David Faure
 


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

Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.

[Matthew Dawson]

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


Everything looked good, but when I tried to rerun the tests to make sure 
everything is ok the kconfig_in_global_object test I'm getting an abort with 
the same error message.  Backtrace:
```
#5  0x7791b630 in qAppName () at kernel/qcoreapplication.cpp:541
#6  0x778a9c76 in QLockFilePrivate::tryLock_sys 
(this=this@entry=0x608ec0) at io/qlockfile_unix.cpp:140
#7  0x7783c1ec in QLockFile::tryLock (this=optimized out, timeout=-1) 
at io/qlockfile.cpp:208
#8  0x77ba17db in KConfigIniBackend::lock (this=0x608fe0) at 
/home/matthew/kde/kconfig/src/core/kconfigini.cpp:634
#9  0x77b84ce0 in KConfigPrivate::lockLocal (this=0x608b40) at 
/home/matthew/kde/kconfig/src/core/kconfig.cpp:104
#10 0x77b86aa9 in KConfig::sync (this=0x606ae0) at 
/home/matthew/kde/kconfig/src/core/kconfig.cpp:415
#11 0x77b85c28 in KConfig::~KConfig (this=0x606ae0, 
__in_chrg=optimized out) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:267
#12 0x77b85c82 in KConfig::~KConfig (this=0x606ae0, 
__in_chrg=optimized out) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:270
#13 0x00401844 in Tester::~Tester (this=0x6030e8 (anonymous 
namespace)::Q_QGS_globalTestObject::innerFunction()::holder, 
__in_chrg=optimized out)
at /home/matthew/kde/kconfig/autotests/kconfig_in_global_object.cpp:56
```
Which seems to be second issue we found.  I'm not sure how to solve this issue 
yet, but I'd be happy to just document the issue and strike this test.

I only get this when the configuration file does not exist before the program 
executes, which may be why we missed it from the earlier patches.

- Matthew Dawson


On Feb. 21, 2015, 2:58 p.m., David Faure wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122232/
 ---
 
 (Updated Feb. 21, 2015, 2:58 p.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 kconfig_in_global_object.cpp comes from kdelibs4support
 (after porting to Q_GLOBAL_STATIC)
 
 ksharedconfig_in_global_object.cpp is now in kdelibs4 too
 (where it works) and reproduces Albert's KgDifficulty testcase.
 
 
 Diffs
 -
 
   autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace 
   autotests/kconfig_in_global_object.cpp PRE-CREATION 
   autotests/ksharedconfig_in_global_object.cpp PRE-CREATION 
   src/core/kconfig.cpp 1da816faea5548d2f529755d6538a142a2193720 
   src/core/ksharedconfig.h d0791e461804d421010bc5f82873371a4d3bc992 
 
 Diff: https://git.reviewboard.kde.org/r/122232/diff/
 
 
 Testing
 ---
 
 Both tests pass and the QCoreApplication::arguments warning (because called 
 after qApp is destroyed) is gone.
 
 
 Thanks,
 
 David Faure
 


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

Re: Extra patch for KConfig 5.7 release

[Matthew Dawson]

On February 14, 2015 11:16:42 AM David Faure wrote:
 This is rather a behavior-incompatible change (and another one to revert).
 So, to avoid the packagers killing me for updating 5.7 two days *after* the
 supposed public release day (*), I would rather make a 5.7.1 kconfig release
 with this commit in.
 In practice users will only get a few days (at most) with 5.7.0 and broken
 kconf_update behaviour - a runtime bug, it happens.
 
 (*) I forgot to make the tarballs public so I was about to do so right now,
 but they surely made their packages public already.
Ok, sounds good.  I didn't see the tarballs/tags, so I wasn't sure if the 
release had happened already.  I'll try to be better about such things in the 
future.

Thanks,
-- 
Matthew

smime.p7s
Description: S/MIME cryptographic signature
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Extra patch for KConfig 5.7 release

[Matthew Dawson]

Hi all,

If it is not too late to sneak an extra patch into KConfig's 5.7 release, 
could commit 9eee15917e01a89d937d1cba2eebbe9d65daeb72 ( 
http://commits.kde.org/kconfig/9eee15917e01a89d937d1cba2eebbe9d65daeb72 ) be 
added in?

This change reverts a SIC, which will break updating of configuration files 
using kconf_update.  The underlying bug from the original commit is still a 
problem after this revert, and I had hoped to tackle the entire issue before 
5.7.  Since that didn't get done, I rather fix the SIC for 5.7 then break 
existing software.

The change is only into a couple lines + documentation and tests.  It is 
relatively straight forward, and shouldn't cause any regressions (Jenkins 
remains green).

Thanks,
-- 
Matthew

smime.p7s
Description: S/MIME cryptographic signature
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: strange problem with KConfig

[Matthew Dawson]

On February 5, 2015 08:49:16 PM Marco Martin wrote:
 Hi all,
 I was investigating this bug..
 https://bugs.kde.org/show_bug.cgi?id43583

 but after digging and digging, it looks like more and more an issue in
 KConfig..
 basically if there is a config file in a system-wide location (to give
 default configs that would then be overridden by whatever other value in
 ~/.config)
I've investigated this further, and it does appear to be a bug in KConfig.  I
did some reductions on the look and feel kcm, to find what code responsible.
It appears the foreach loop in krdb.cpp on line 683 triggers the funny writes.
As that loop doesn't do anything odd, KConfig is definitely responsible.
Hopefully this can be reduced down to a test case and then fixed.

However, I think the bug can be worked around in the short term for the kcm by
having the KSharedConfigs in question use only the file they are pointed at.
I think that is actually the correct behaviour, as those files are not
supposed to use the global or system configuration files.  I've attached a
(relatively untested) sample patch.  It fixes the bug, but I'm not sure if it
allows the kcm to continue functioning as expected.
--
Matthewdiff --git a/kcms/krdb/krdb.cpp b/kcms/krdb/krdb.cpp
index 6838b21..5d92019 100644
--- a/kcms/krdb/krdb.cpp
+++ b/kcms/krdb/krdb.cpp
@@ -634,7 +634,7 @@ void runRdb( uint flags )
   return;
   }

-  KConfig kde4config(configFilePath);
+  KConfig kde4config(configFilePath, KConfig::SimpleConfig);

   KConfigGroup kde4generalGroup(kde4config, General);
   kde4generalGroup.writeEntry(ColorScheme, colorSchemeName);
@@ -677,7 +677,7 @@ void runRdb( uint flags )
   kde4IconGroup.sync();

   //copy all the groups in the color scheme in kdeglobals
-  KSharedConfigPtr kde4ColorConfig = KSharedConfig::openConfig(src);
+  KSharedConfigPtr kde4ColorConfig = KSharedConfig::openConfig(src, KConfig::SimpleConfig);

   foreach (const QString grp, kde4ColorConfig-groupList()) {
   KConfigGroup cg(kde4ColorConfig, grp);


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: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.

[Matthew Dawson]

 On Jan. 27, 2015, 3:55 p.m., Matthew Dawson wrote:
  Unforunately, this cause test system failures in the the 
  kconfigskeletontest test suite.  I'm not sure why this should create issues 
  there.
  
  However, I have a partial solution that avoids creating a full 
  KSharedConfig.  Instead, only globalData needs to be called in KConfig to 
  ensure the object is created as soon as possible (without needing a 
  QCoreApplication).  This should avoid having any other globals created 
  before.
  
  However, this causes a crash later.  The root of the issue is in the 
  KConfigPrivate constructor, line 98.  Creating the QLocale after 
  QCoreApplication is gone is apparently broken as well.  I'm not sure how to 
  solve that, maybe cache the value we need from QLocale, similar to caching 
  the arguments?
  
  Also, I think the copied over KConfig test case really needs to match the 
  KSharedConfig test, as using KSharedConfig can mask the problem since 
  KSharedConfig's pointer is cached, avoiding the KConfig constructor.
 
 David Faure wrote:
 Oh. Indeed. I can explain the test failure: that test was removing the 
 config file and then creating a KConfigSkeleton around it (which was supposed 
 to be the instanciation of the KSharedConfig, starting from no file). With 
 my change, the KSharedConfig already exists before that, we delete the file 
 underneath, but it keeps existing, so the KConfigSkeleton also uses the data 
 from that now-deleted file. It's more or less like refcounting - we kept an 
 old object alive by creating it upfront.
 
 You're right, we could just store the args instead, for a more minimal 
 change.
 But I tried it and I hit the same QLocale issue as you did. Argh.
 We're really doing too much in that global object dtor. Wrapping up is 
 ok, creating new stuff is not.
 
 I tried working around the issue by keeping a KSharedConfig::Ptr data 
 member in the Tester class - which sounds like an extremely good idea for 
 apps to do, in order to reuse it rather than recreate and reparse at shutdown 
 time. This takes care of that issue, but another one then shows up:
 ~Tester - ~KSharedConfig - KConfig::sync - QLockFile::lock - 
 QApplication::qAppName: Please instantiate the QApplication object first.
 We didn't hit that in kdelibs4 either because I wrote QLockFile for Qt5, 
 and KLockFile didn't call qAppName, it used a refcounting KComponentData.
 I guess I could add an if (qApp) in QLockFile, but we're really going 
 into fragile territories. We're definitely not just fixing a regression 
 anymore since kdelibs4 asserted with this testcase.
 
 The KSharedConfig testcase doesn't avoid the KConfig constructor, the way 
 it's written. The caching is refcounting, it only actually caches if there's 
 something storing a Ptr somewhere (hence my suggestion to actually do that, 
 because apps should really do that, especially since they don't have 
 KComponentData doing that for them anymore). I'm fine with any change you 
 want to see in the KConfig testcase though, but let's sort out the main 
 testcase first.
 
 I'm currently thinking Revision 2 of this patch was the best, it fixed 
 the regression compared to kdelibs4 without introducing new issues.

I'm thinking using KConfig when a QCoreApplication is not present should just 
not be supported.  At the very least, the new issue in the save path means you 
can't actually save anything when the global destructors are running, which 
makes using KConfig then pointless.  If we can get QLockFile to work correctly 
in this case, we could revisit this.

For now, I think the best plan is going back to revision 2 (as you suggested) + 
a comment on KConfig/KSharedConfig warning about using them without a valid 
QCoreApplication is not supported.  Does that sound good to you?


- Matthew


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


On Jan. 27, 2015, 3:10 a.m., David Faure wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122232/
 ---
 
 (Updated Jan. 27, 2015, 3:10 a.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 kconfig_in_global_object.cpp comes from kdelibs4support
 (after porting to Q_GLOBAL_STATIC)
 
 ksharedconfig_in_global_object.cpp is new (but works with kdelibs4)
 and reproduces Albert's KgDifficulty testcase.
 
 
 Diffs
 -
 
   autotests/kconfig_in_global_object.cpp PRE-CREATION 
   autotests/ksharedconfig_in_global_object.cpp PRE-CREATION 
   src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a 
   src/core/ksharedconfig.cpp

Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.

[Matthew Dawson]

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


Unforunately, this cause test system failures in the the kconfigskeletontest 
test suite.  I'm not sure why this should create issues there.

However, I have a partial solution that avoids creating a full KSharedConfig.  
Instead, only globalData needs to be called in KConfig to ensure the object is 
created as soon as possible (without needing a QCoreApplication).  This should 
avoid having any other globals created before.

However, this causes a crash later.  The root of the issue is in the 
KConfigPrivate constructor, line 98.  Creating the QLocale after 
QCoreApplication is gone is apparently broken as well.  I'm not sure how to 
solve that, maybe cache the value we need from QLocale, similar to caching the 
arguments?

Also, I think the copied over KConfig test case really needs to match the 
KSharedConfig test, as using KSharedConfig can mask the problem since 
KSharedConfig's pointer is cached, avoiding the KConfig constructor.

- Matthew Dawson


On Jan. 27, 2015, 3:10 a.m., David Faure wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122232/
 ---
 
 (Updated Jan. 27, 2015, 3:10 a.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 kconfig_in_global_object.cpp comes from kdelibs4support
 (after porting to Q_GLOBAL_STATIC)
 
 ksharedconfig_in_global_object.cpp is new (but works with kdelibs4)
 and reproduces Albert's KgDifficulty testcase.
 
 
 Diffs
 -
 
   autotests/kconfig_in_global_object.cpp PRE-CREATION 
   autotests/ksharedconfig_in_global_object.cpp PRE-CREATION 
   src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a 
   src/core/ksharedconfig.cpp e059b87a1cc1df50693a668ef791e7a61050ef88 
   autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace 
 
 Diff: https://git.reviewboard.kde.org/r/122232/diff/
 
 
 Testing
 ---
 
 Both tests pass and the QCoreApplication::arguments warning (because called 
 after qApp is destroyed) is gone.
 
 
 Thanks,
 
 David Faure
 


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

Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.

[Matthew Dawson]

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


Sorry, I just discovered an issue with this change as is.  If the global object 
is created before KConfig's Global object, a seg fault develops.  In this case, 
KConfig's global is created first avoiding the pain.  However, commenting out 
the noted line causes a seg fault in the tests.

I don't think we can assume people will know to create appropriate KConfig 
objects, so I'd like to avoid relying on this in the test to pass.  I'm not 
sure how we can force the creation of the global ahead of time.  My best 
thought would be some sort of global pointer to the QStringList, combined with 
some atomic pointer operations to create it.  Thoughts?

Otherwise, everything looked go to me.


autotests/ksharedconfig_in_global_object.cpp
https://git.reviewboard.kde.org/r/122232/#comment51847

Line to comment out.  For the final version this should work without this 
line.


- Matthew Dawson


On Jan. 26, 2015, 3:11 a.m., David Faure wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122232/
 ---
 
 (Updated Jan. 26, 2015, 3:11 a.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 kconfig_in_global_object.cpp comes from kdelibs4support
 (after porting to Q_GLOBAL_STATIC)
 
 ksharedconfig_in_global_object.cpp is new (but works with kdelibs4)
 and reproduces Albert's KgDifficulty testcase.
 
 
 Diffs
 -
 
   autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace 
   autotests/kconfig_in_global_object.cpp PRE-CREATION 
   autotests/ksharedconfig_in_global_object.cpp PRE-CREATION 
   src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a 
 
 Diff: https://git.reviewboard.kde.org/r/122232/diff/
 
 
 Testing
 ---
 
 Both tests pass and the QCoreApplication::arguments warning (because called 
 after qApp is destroyed) is gone.
 
 
 Thanks,
 
 David Faure
 


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

Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.

[Matthew Dawson]

 On Jan. 24, 2015, 12:12 p.m., Matthew Dawson wrote:
  autotests/kconfig_in_global_object.cpp, line 56
  https://git.reviewboard.kde.org/r/122232/diff/1/?file=33#file33line56
 
  This doesn't actually trigger anything when run without the fix below.  
  Since a name is provided in initConfig, it never tries to fetch the global 
  name and thus doesn't test the issue.
  
  I think these two files should be the exact same, except for testing 
  KSharedConfig vs KConfig.
 
 David Faure wrote:
 Yes this unittest is not directly related to the change. It's just that I 
 found it in kdelibs4support and brought it over. Any test is good to have, 
 right?

I'd prefer to have a specific case for this, as it seems the test using 
ksharedconfig does the same thing anyways.  The only difference is that the 
configuration is created before QCoreApplication quits.  Since it's probably a 
good idea to ensure that works before worring about the argument copying, lets 
keep this test.  Just two things: I don't think line 45 is necessary, since t 
is in fact used on line 49.  Also, please add the environment variable, similar 
to below, to verify it doesn't complain.


 On Jan. 24, 2015, 12:12 p.m., Matthew Dawson wrote:
  src/core/kconfig.cpp, line 542
  https://git.reviewboard.kde.org/r/122232/diff/1/?file=35#file35line542
 
  Reading the Qt documentation, I think its possible that appArgs[0] 
  isn't the application name on Windows, and thus appArgs may stay empty.
  
  Maybe add another conditional checking its empty in this if statement, 
  and if it still is add some random value?
 
 David Faure wrote:
 It's possible that appArgs[0] is not the app name, but I'm pretty sure 
 the QStringList we get from QCoreApplication is never empty.

I don't have a Windows machine to test this theory on.  Could you forward this 
commit to the KDE Windows developers, so they can double check and fix as 
necessary?  Since this solves the bug on *nix platforms, and should mostly 
solve it on Windows, I don't think blocking this version over this issue is 
worthwhile then.


- Matthew


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


On Jan. 23, 2015, 5:39 p.m., David Faure wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122232/
 ---
 
 (Updated Jan. 23, 2015, 5:39 p.m.)
 
 
 Review request for KDE Frameworks, Albert Astals Cid and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 kconfig_in_global_object.cpp comes from kdelibs4support
 (after porting to Q_GLOBAL_STATIC)
 
 ksharedconfig_in_global_object.cpp is new (but works with kdelibs4)
 and reproduces Albert's KgDifficulty testcase.
 
 
 Diffs
 -
 
   autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace 
   autotests/kconfig_in_global_object.cpp PRE-CREATION 
   autotests/ksharedconfig_in_global_object.cpp PRE-CREATION 
   src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a 
 
 Diff: https://git.reviewboard.kde.org/r/122232/diff/
 
 
 Testing
 ---
 
 Both tests pass and the QCoreApplication::arguments warning (because called 
 after qApp is destroyed) is gone.
 
 
 Thanks,
 
 David Faure
 


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

Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.

[Matthew Dawson]

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


The overall idea is sound.  Just a few issues:


autotests/kconfig_in_global_object.cpp
https://git.reviewboard.kde.org/r/122232/#comment51750

This doesn't actually trigger anything when run without the fix below.  
Since a name is provided in initConfig, it never tries to fetch the global name 
and thus doesn't test the issue.

I think these two files should be the exact same, except for testing 
KSharedConfig vs KConfig.



autotests/ksharedconfig_in_global_object.cpp
https://git.reviewboard.kde.org/r/122232/#comment51751

Just before this, could you stick a:
qputenv(QT_FATAL_WARNINGS, 1);

That way if a warning is printed the test will fail.  Otherwise this change 
might be removed with tests still passing.



src/core/kconfig.cpp
https://git.reviewboard.kde.org/r/122232/#comment51752

Reading the Qt documentation, I think its possible that appArgs[0] isn't 
the application name on Windows, and thus appArgs may stay empty.

Maybe add another conditional checking its empty in this if statement, and 
if it still is add some random value?


- Matthew Dawson


On Jan. 23, 2015, 5:39 p.m., David Faure wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122232/
 ---
 
 (Updated Jan. 23, 2015, 5:39 p.m.)
 
 
 Review request for KDE Frameworks, Albert Astals Cid and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 kconfig_in_global_object.cpp comes from kdelibs4support
 (after porting to Q_GLOBAL_STATIC)
 
 ksharedconfig_in_global_object.cpp is new (but works with kdelibs4)
 and reproduces Albert's KgDifficulty testcase.
 
 
 Diffs
 -
 
   autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace 
   autotests/kconfig_in_global_object.cpp PRE-CREATION 
   autotests/ksharedconfig_in_global_object.cpp PRE-CREATION 
   src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a 
 
 Diff: https://git.reviewboard.kde.org/r/122232/diff/
 
 
 Testing
 ---
 
 Both tests pass and the QCoreApplication::arguments warning (because called 
 after qApp is destroyed) is gone.
 
 
 Thanks,
 
 David Faure
 


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

Re: Review Request 122206: [kio] Make tests optional

[Matthew Dawson]

 On Jan. 23, 2015, 8:43 a.m., Vishesh Handa wrote:
  I'm not against this, but I am curious as to why this is being done. 
  
  I would think that packagers should be building the tests and running them 
  on their platform and make sure everything passes. We have a strict policy 
  that all tests must always pass.

This is mostly useful on source based distributions (specifically, this patch 
comes from Gentoo).  While in general running tests everywhere would be great, 
source distro users may not have the cpu time to compile/run tests.  Also, some 
test suites don't work and users may not care to figure out why (for instance, 
last time I tried enabling tests in Gentoo, binutils failed its suite).

For binary distriubtions, I agree they should be running tests (especially 
since we work to keep them green).  But source based distros aren't so clear 
cut.


- Matthew


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


On Jan. 22, 2015, 2:48 p.m., Andreas Sturmlechner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122206/
 ---
 
 (Updated Jan. 22, 2015, 2:48 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kio
 
 
 Description
 ---
 
 [kio] Make tests optional
 This is a small patch to CMakeLists.txt to only depend on Qt5Test if 
 BUILD_TESTING.
 
 
 Diffs
 -
 
   CMakeLists.txt 7fe0be5d4b2d7d9475a7844b4f8d93fc2f0a00c3 
 
 Diff: https://git.reviewboard.kde.org/r/122206/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Andreas Sturmlechner
 


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

Re: Review Request 122206: [kio] Make tests optional

[Matthew Dawson]

 On Jan. 23, 2015, 8:43 a.m., Vishesh Handa wrote:
  I'm not against this, but I am curious as to why this is being done. 
  
  I would think that packagers should be building the tests and running them 
  on their platform and make sure everything passes. We have a strict policy 
  that all tests must always pass.
 
 Matthew Dawson wrote:
 This is mostly useful on source based distributions (specifically, this 
 patch comes from Gentoo).  While in general running tests everywhere would be 
 great, source distro users may not have the cpu time to compile/run tests.  
 Also, some test suites don't work and users may not care to figure out why 
 (for instance, last time I tried enabling tests in Gentoo, binutils failed 
 its suite).
 
 For binary distriubtions, I agree they should be running tests 
 (especially since we work to keep them green).  But source based distros 
 aren't so clear cut.
 
 Andreas Sturmlechner wrote:
 Exactly, packagers do build the tests of course, but that does not mean 
 users of source packages should have a permanent dependency on Qt5Test.
 
 Vishesh Handa wrote:
 It is even more important for source based distros to be running tests. 
 They generally have very different compile options and flags. What is the 
 point of them running the software and possibly finding bugs, when it could 
 have been caught by just running the tests.
 
 Actually, the more I think about this, the more I realize that everyone 
 should be running the autotests. -2 from my side. But I'm not the maintainer 
 of kio.
 
 Vishesh Handa wrote:
  Exactly, packagers do build the tests of course, but that does not mean 
 users of source packages should have a permanent dependency on Qt5Test.
 
 On a source based distribution you already have a dependnecy on cmake, 
 the compiler, and many other things. These are only required during build 
 time. Qt5Test is the same. Once the pacakge has been built + tests have been 
 run, Qt5Test can be removed.
 
 Matthew Dawson wrote:
 At least on Gentoo, by default build time dependencies are not 
 automatically removed (though you can remove them if you want).  Generally 
 speaking that is the right choice, as you will need cmake/compiler/etc. later 
 to build the package when a version is released.  Also, one of the benefits 
 of Gentoo is that the entire development toolchain sticks around, allowing 
 for easy development/bug triaging.
 
 Anyways, source based distros won't always run tests, because users won't 
 always want to run them.  In a perfect world, I agree that is wrong.  In 
 reality, I don't run any test suites across any of my Gentoo installs.  So 
 forcing the tests to build just burns CPU time, and is easily patched out by 
 downstreams.  I don't think trying to force this will get KDE anywhere.
 
 Vishesh Handa wrote:
  Anyways, source based distros won't always run tests, because users 
 won't always want to run them.  In a perfect world, I agree that is wrong.  
 In reality, I don't run any test suites across any of my Gentoo installs.  So 
 forcing the tests to build just burns CPU time, and is easily patched out by 
 downstreams.  I don't think trying to force this will get KDE anywhere.
 
 - If the user doesn't want to run them, I'm sure Gentoo can provide some 
 options for that. Compiling them cannot be such a huge cost.
 - They are already burning CPU time by using a source based distro. This 
 way they might actually catch some bugs and possibly not waste developers 
 time by filing bugs which may have been an issue with their system.
 
 I'm not sure if I will approve such patches on packages I maintain.
 
 Matthew Dawson wrote:
 I think we've both stated our piece here, and we aren't going to get 
 further towards a consensus.  May I suggest posting this to the general 
 kde-frameworks (or kde-core-devel, I'm not sure what would be better) seeking 
 to make a general policy wrt Frameworks?  That way Frameworks has a 
 consistent approach to building tests, whatever way the community decides.
 
 In the meantime we should probably merge this patch, as building 
 autotests without finding Qt5Test is only going to break builds.  Then 
 packages can be updated with the policy decision by removing the 
 BUILD_TESTING option.
 
 The Plasma community should also probably come to a consensus for its 
 packages as well.
 
 Albert Astals Cid wrote:
 Your logic is flawed, you say  building autotests without finding 
 Qt5Test is only going to break builds.
 
 That is correct, just that Qt5Test is being searched for, so your 
 rationale for applying the patch is moot.

Sorry, I misread the patch.  You are correct the build is fine as is.  We 
should discuss this on the mailing list first before applying.


- Matthew


---
This is an automatically generated e-mail. To reply, visit:
https

Re: Review Request 122206: [kio] Make tests optional

[Matthew Dawson]

 On Jan. 23, 2015, 8:43 a.m., Vishesh Handa wrote:
  I'm not against this, but I am curious as to why this is being done. 
  
  I would think that packagers should be building the tests and running them 
  on their platform and make sure everything passes. We have a strict policy 
  that all tests must always pass.
 
 Matthew Dawson wrote:
 This is mostly useful on source based distributions (specifically, this 
 patch comes from Gentoo).  While in general running tests everywhere would be 
 great, source distro users may not have the cpu time to compile/run tests.  
 Also, some test suites don't work and users may not care to figure out why 
 (for instance, last time I tried enabling tests in Gentoo, binutils failed 
 its suite).
 
 For binary distriubtions, I agree they should be running tests 
 (especially since we work to keep them green).  But source based distros 
 aren't so clear cut.
 
 Andreas Sturmlechner wrote:
 Exactly, packagers do build the tests of course, but that does not mean 
 users of source packages should have a permanent dependency on Qt5Test.
 
 Vishesh Handa wrote:
 It is even more important for source based distros to be running tests. 
 They generally have very different compile options and flags. What is the 
 point of them running the software and possibly finding bugs, when it could 
 have been caught by just running the tests.
 
 Actually, the more I think about this, the more I realize that everyone 
 should be running the autotests. -2 from my side. But I'm not the maintainer 
 of kio.
 
 Vishesh Handa wrote:
  Exactly, packagers do build the tests of course, but that does not mean 
 users of source packages should have a permanent dependency on Qt5Test.
 
 On a source based distribution you already have a dependnecy on cmake, 
 the compiler, and many other things. These are only required during build 
 time. Qt5Test is the same. Once the pacakge has been built + tests have been 
 run, Qt5Test can be removed.
 
 Matthew Dawson wrote:
 At least on Gentoo, by default build time dependencies are not 
 automatically removed (though you can remove them if you want).  Generally 
 speaking that is the right choice, as you will need cmake/compiler/etc. later 
 to build the package when a version is released.  Also, one of the benefits 
 of Gentoo is that the entire development toolchain sticks around, allowing 
 for easy development/bug triaging.
 
 Anyways, source based distros won't always run tests, because users won't 
 always want to run them.  In a perfect world, I agree that is wrong.  In 
 reality, I don't run any test suites across any of my Gentoo installs.  So 
 forcing the tests to build just burns CPU time, and is easily patched out by 
 downstreams.  I don't think trying to force this will get KDE anywhere.
 
 Vishesh Handa wrote:
  Anyways, source based distros won't always run tests, because users 
 won't always want to run them.  In a perfect world, I agree that is wrong.  
 In reality, I don't run any test suites across any of my Gentoo installs.  So 
 forcing the tests to build just burns CPU time, and is easily patched out by 
 downstreams.  I don't think trying to force this will get KDE anywhere.
 
 - If the user doesn't want to run them, I'm sure Gentoo can provide some 
 options for that. Compiling them cannot be such a huge cost.
 - They are already burning CPU time by using a source based distro. This 
 way they might actually catch some bugs and possibly not waste developers 
 time by filing bugs which may have been an issue with their system.
 
 I'm not sure if I will approve such patches on packages I maintain.

I think we've both stated our piece here, and we aren't going to get further 
towards a consensus.  May I suggest posting this to the general kde-frameworks 
(or kde-core-devel, I'm not sure what would be better) seeking to make a 
general policy wrt Frameworks?  That way Frameworks has a consistent approach 
to building tests, whatever way the community decides.

In the meantime we should probably merge this patch, as building autotests 
without finding Qt5Test is only going to break builds.  Then packages can be 
updated with the policy decision by removing the BUILD_TESTING option.

The Plasma community should also probably come to a consensus for its packages 
as well.


- Matthew


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


On Jan. 22, 2015, 2:48 p.m., Andreas Sturmlechner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122206/
 ---
 
 (Updated Jan. 22, 2015, 2:48 p.m.)
 
 
 Review request for KDE

Re: Review Request 122206: [kio] Make tests optional

[Matthew Dawson]

 On Jan. 23, 2015, 8:43 a.m., Vishesh Handa wrote:
  I'm not against this, but I am curious as to why this is being done. 
  
  I would think that packagers should be building the tests and running them 
  on their platform and make sure everything passes. We have a strict policy 
  that all tests must always pass.
 
 Matthew Dawson wrote:
 This is mostly useful on source based distributions (specifically, this 
 patch comes from Gentoo).  While in general running tests everywhere would be 
 great, source distro users may not have the cpu time to compile/run tests.  
 Also, some test suites don't work and users may not care to figure out why 
 (for instance, last time I tried enabling tests in Gentoo, binutils failed 
 its suite).
 
 For binary distriubtions, I agree they should be running tests 
 (especially since we work to keep them green).  But source based distros 
 aren't so clear cut.
 
 Andreas Sturmlechner wrote:
 Exactly, packagers do build the tests of course, but that does not mean 
 users of source packages should have a permanent dependency on Qt5Test.
 
 Vishesh Handa wrote:
 It is even more important for source based distros to be running tests. 
 They generally have very different compile options and flags. What is the 
 point of them running the software and possibly finding bugs, when it could 
 have been caught by just running the tests.
 
 Actually, the more I think about this, the more I realize that everyone 
 should be running the autotests. -2 from my side. But I'm not the maintainer 
 of kio.
 
 Vishesh Handa wrote:
  Exactly, packagers do build the tests of course, but that does not mean 
 users of source packages should have a permanent dependency on Qt5Test.
 
 On a source based distribution you already have a dependnecy on cmake, 
 the compiler, and many other things. These are only required during build 
 time. Qt5Test is the same. Once the pacakge has been built + tests have been 
 run, Qt5Test can be removed.

At least on Gentoo, by default build time dependencies are not automatically 
removed (though you can remove them if you want).  Generally speaking that is 
the right choice, as you will need cmake/compiler/etc. later to build the 
package when a version is released.  Also, one of the benefits of Gentoo is 
that the entire development toolchain sticks around, allowing for easy 
development/bug triaging.

Anyways, source based distros won't always run tests, because users won't 
always want to run them.  In a perfect world, I agree that is wrong.  In 
reality, I don't run any test suites across any of my Gentoo installs.  So 
forcing the tests to build just burns CPU time, and is easily patched out by 
downstreams.  I don't think trying to force this will get KDE anywhere.


- Matthew


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


On Jan. 22, 2015, 2:48 p.m., Andreas Sturmlechner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122206/
 ---
 
 (Updated Jan. 22, 2015, 2:48 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kio
 
 
 Description
 ---
 
 [kio] Make tests optional
 This is a small patch to CMakeLists.txt to only depend on Qt5Test if 
 BUILD_TESTING.
 
 
 Diffs
 -
 
   CMakeLists.txt 7fe0be5d4b2d7d9475a7844b4f8d93fc2f0a00c3 
 
 Diff: https://git.reviewboard.kde.org/r/122206/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Andreas Sturmlechner
 


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

Re: Review Request 121908: Fix unit test failure on machines with an empty ~/.qttest.

[Matthew Dawson]

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

(Updated Jan. 8, 2015, 8:16 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Repository: frameworkintegration


Description
---

To execute the KdePlatformTheme and the KStyle unit tests, a local
kdeglobals test file is required inside the Qt test folders.  If the Qt
test folders don't exist when the test executable starts, this copy will
silently fail, causing further failures.

Now, attempt to pre-create this folder before the copy.  Also abort
the test if the folder creation or the file copy fail, to help diagnose
this issue in the future.


Diffs
-

  autotests/kdeplatformtheme_unittest.cpp 
cc17ef6e3f3c0db3c4597105b32320f0aeb52b0f 
  autotests/kstyle_unittest.cpp e0e0046100acc195b1a3c36bbbe67e5861d7b7ee 

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


Testing
---

Executing the test suite locally now succeeds.


Thanks,

Matthew Dawson

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

Review Request 121908: Fix unit test failure on machines with an empty ~/.qttest.

[Matthew Dawson]

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

Review request for KDE Frameworks.


Repository: frameworkintegration


Description
---

To execute the KdePlatformTheme and the KStyle unit tests, a local
kdeglobals test file is required inside the Qt test folders.  If the Qt
test folders don't exist when the test executable starts, this copy will
silently fail, causing further failures.

Now, attempt to pre-create this folder before the copy.  Also abort
the test if the folder creation or the file copy fail, to help diagnose
this issue in the future.


Diffs
-

  autotests/kdeplatformtheme_unittest.cpp 
cc17ef6e3f3c0db3c4597105b32320f0aeb52b0f 
  autotests/kstyle_unittest.cpp e0e0046100acc195b1a3c36bbbe67e5861d7b7ee 

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


Testing
---

Executing the test suite locally now succeeds.


Thanks,

Matthew Dawson

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

Re: Review Request 121838: Fix KCoreConfigSkeleton when toggling a value with saves in between

[Matthew Dawson]

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

Ship it!


Thanks, LGTM.  If possible, could you please split the unit test refactoring 
out to a separate commit just before the bug fix commit?  If not, just push it 
as is.

- Matthew Dawson


On Jan. 6, 2015, 5:41 p.m., Albert Astals Cid wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121838/
 ---
 
 (Updated Jan. 6, 2015, 5:41 p.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 We need to refresh mLoadedValue after a save, otherwise the value is stale.
 
 I'm doing this by resetting mLoadedValue to mReference in all the 
 ::writeConfig.
 
 I've also refactored the tests so they can be run independently now just fine.
 
 
 Diffs
 -
 
   autotests/kconfigskeletontest.cpp f401b9f 
   src/core/kcoreconfigskeleton.h f8313d1 
   src/core/kcoreconfigskeleton.cpp e4255a6 
   CMakeLists.txt 205c38b 
   autotests/kconfigskeletontest.h c54c7b0 
 
 Diff: https://git.reviewboard.kde.org/r/121838/diff/
 
 
 Testing
 ---
 
 My tests now pass.
 
 
 Thanks,
 
 Albert Astals Cid
 


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

Re: Review Request 121838: Fix KCoreConfigSkeleton when toggling a value with saves in between

[Matthew Dawson]

 On Jan. 4, 2015, 12:02 p.m., David Faure wrote:
  This undoes 4846b50aea0bc2262238963a85ab3556c22412e4 
  (https://git.reviewboard.kde.org/r/117010/), basically.
  
  However looking back at the discussions with Alexander Richardson, this 
  might be only a revert of the part that went further than what *I* was 
  asking for ;) My problem was that save called reparseConfiguration (see 
  7af829a341c1ff04f9499336a28b6a4dd20bdbdc). But nowadays read which doesn't 
  call reparseConfiguration (right?), so maybe it's fine to call it from 
  save. I'll let Alexander remind us why he removed the read call.
 
 Matthew Dawson wrote:
 The read call was removed to avoid changing the KCoreConfigSekleton 
 during the save call, as it wasn't documented as doing that and may surprise 
 some people that flushing their changes may load other unrelated changes.  I 
 would prefer to keep that, for the same reason.
 
 I do agree there is a bug that needs fixing, I'm just not sure how to fix 
 it.  As the API stands now, the check with mLoadValue only works if the 
 KConfig used with the KCoreConfigSkeletonItem doesn't change (never mind 
 people manually calling those functions).  Otherwise, the value of mReference 
 gets out of sync with KConfig, and breaks saving in general.  It appears 
 removing the mLoadValue variable solves the bug, and avoids changing the 
 underlying KConfig, assuming its loaded value hasn't changed.  The only 
 benfit I see of keeping the check is avoiding the more complicated checks 
 carried out by KConfig to verify the value is unchanged.
 
 I think the best course of action is to remove the check for now, as it 
 create subtle issues.  The way KCoreConfigSkeletonItem works may need to be 
 changed, but that can be done later.  Thoughts?
 
 Albert Astals Cid wrote:
 I disagree, the easiest thing is adding back the read, it's been there in 
 kdelibs for years, it works and has been tested by the time. Users can't be 
 suprised by it, since it's been there forever. OTOH yes, it changes the 
 KCoreConfigSekleton because someone added the possibility of modifying it, 
 like the removeItem and stuff, before it was all static and fine. 
 
 Honestly before removing the mLoadedValue variable i think i'd prefer 
 adding mLoadedValue = mReference; in all the ::writeConfig

Regarding surprise, yes an existing user is unlikely to be surprised as they 
have probably been using the api and figured out its warts.  I was firstly 
thinking more of new users coming to KConfig, who read the documentation and 
don't learn of the fact the value can change when saving (easily fixed with a 
documentation string).  Secondly, I'd expect most people to expect the save 
call to not modify the values in any way, but that's my belief and can be 
changed.  Due to point two, I'd prefer not to put the read call back in the 
save path just to fix this bug, but I'm happy to reconsider as a general aspect 
of the code.

I'm not sold on removing either mLoadedValue, as that means KEntryMap can be 
modified when it isn't expected.  If you think adding that extra line in all 
the saves paths is better then removing mLoadedValue checks, I think that is 
probably the best route for now.  Ideally I think mLoadedValue should be tied 
to the KConfig used, but I don't see an easy way to do that while maintaining 
binary compatibility so I'm not worried about that to fix this bug.


- Matthew


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


On Jan. 4, 2015, 11:04 a.m., Albert Astals Cid wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121838/
 ---
 
 (Updated Jan. 4, 2015, 11:04 a.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 We need to refresh mLoadedValue after a save, otherwise the value is stale.
 
 I'm doing this by adding back the read() call in KCoreConfigSkeleton::save 
 (which is what kdelibs did).
 
 Another option would be adding lots of mLoadedValue = mReference; in all the 
 ::writeConfig, but that seems much more prone.
 
 I've also refactored the tests so they can be run independently now just fine.
 
 
 Diffs
 -
 
   autotests/kconfigskeletontest.h c54c7b0 
   autotests/kconfigskeletontest.cpp f401b9f 
   src/core/kcoreconfigskeleton.cpp e4255a6 
 
 Diff: https://git.reviewboard.kde.org/r/121838/diff/
 
 
 Testing
 ---
 
 My tests now pass.
 
 
 Thanks,
 
 Albert Astals Cid
 


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

Re: Review Request 121838: Fix KCoreConfigSkeleton when toggling a value with saves in between

[Matthew Dawson]

 On Jan. 4, 2015, 12:02 p.m., David Faure wrote:
  This undoes 4846b50aea0bc2262238963a85ab3556c22412e4 
  (https://git.reviewboard.kde.org/r/117010/), basically.
  
  However looking back at the discussions with Alexander Richardson, this 
  might be only a revert of the part that went further than what *I* was 
  asking for ;) My problem was that save called reparseConfiguration (see 
  7af829a341c1ff04f9499336a28b6a4dd20bdbdc). But nowadays read which doesn't 
  call reparseConfiguration (right?), so maybe it's fine to call it from 
  save. I'll let Alexander remind us why he removed the read call.

The read call was removed to avoid changing the KCoreConfigSekleton during the 
save call, as it wasn't documented as doing that and may surprise some people 
that flushing their changes may load other unrelated changes.  I would prefer 
to keep that, for the same reason.

I do agree there is a bug that needs fixing, I'm just not sure how to fix it.  
As the API stands now, the check with mLoadValue only works if the KConfig used 
with the KCoreConfigSkeletonItem doesn't change (never mind people manually 
calling those functions).  Otherwise, the value of mReference gets out of sync 
with KConfig, and breaks saving in general.  It appears removing the mLoadValue 
variable solves the bug, and avoids changing the underlying KConfig, assuming 
its loaded value hasn't changed.  The only benfit I see of keeping the check is 
avoiding the more complicated checks carried out by KConfig to verify the value 
is unchanged.

I think the best course of action is to remove the check for now, as it create 
subtle issues.  The way KCoreConfigSkeletonItem works may need to be changed, 
but that can be done later.  Thoughts?


- Matthew


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


On Jan. 4, 2015, 11:04 a.m., Albert Astals Cid wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121838/
 ---
 
 (Updated Jan. 4, 2015, 11:04 a.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 We need to refresh mLoadedValue after a save, otherwise the value is stale.
 
 I'm doing this by adding back the read() call in KCoreConfigSkeleton::save 
 (which is what kdelibs did).
 
 Another option would be adding lots of mLoadedValue = mReference; in all the 
 ::writeConfig, but that seems much more prone.
 
 I've also refactored the tests so they can be run independently now just fine.
 
 
 Diffs
 -
 
   autotests/kconfigskeletontest.h c54c7b0 
   autotests/kconfigskeletontest.cpp f401b9f 
   src/core/kcoreconfigskeleton.cpp e4255a6 
 
 Diff: https://git.reviewboard.kde.org/r/121838/diff/
 
 
 Testing
 ---
 
 My tests now pass.
 
 
 Thanks,
 
 Albert Astals Cid
 


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

Re: Review Request 121372: Differeniate between bookmarks for documents and the web

[Matthew Dawson]

 On Dec. 8, 2014, 3:40 p.m., Albert Astals Cid wrote:
  I don't think this makes sense at a framework level. If some application is 
  so special that really needs a special case, they can call setText over the 
  kaction themselves, otherwise we're going to end up havin 20 different Add 
  Bookmark for all the minor technicalities of over what it is applied.
 
 Gun Chleoc wrote:
 I'm the person who originally filed the bug. Since I'm just translating 
 and new to KDE, can you give me a recommendation if I should translate this 
 as bookmarks or webmarks then? I don't know if web or document 
 applications are more frequent in KDE, and we should minimize the number of 
 applications that I will have to file bugs with. This is a linguistic issue 
 rather than an application being special - English is just more ambiguous 
 than my language in this case.
 
 Albert Astals Cid wrote:
 You should translate bookmark. If you need bookmark in a web application 
 to be translatable different than bookmark, then file a bug agains that 
 application and ask them to. I am unconvinced a framework has to support all 
 the combinatorials of application use of bookmark multiplied by the various 
 languages that can have different translations for the various uses of 
 bookrmak in various cases.

Based on these comments then, I'm going to discard this RR and close the bug as 
WONTFIX with a pointer back to here.  Thanks for the help!


- Matthew


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


On Dec. 6, 2014, 3:49 p.m., Matthew Dawson wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121372/
 ---
 
 (Updated Dec. 6, 2014, 3:49 p.m.)
 
 
 Review request for KDE Frameworks, Localization and Translation (l10n) and 
 Matthew Dawson.
 
 
 Bugs: 341279
 https://bugs.kde.org/show_bug.cgi?id=341279
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 As requested in bug #1164383, this adds a new standard shortcut
 for adding bookmarks for web pages.
 
 BUG: 1164383
 
 I'm not sure if this is the best approach.  If this is accpted, I'll commit a 
 similar change into KConfigWidgets.
 
 
 Diffs
 -
 
   src/gui/kstandardshortcut.h 5bb07fb9f060b0d5950aed251da985ce3aa46661 
   src/gui/kstandardshortcut.cpp 84007374c2836c1d61cb9b9361bd4217aa4ddc32 
 
 Diff: https://git.reviewboard.kde.org/r/121372/diff/
 
 
 Testing
 ---
 
 Compiles, verified context extracted properly into pot file.
 
 
 Thanks,
 
 Matthew Dawson
 


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

Review Request 121372: Differeniate between bookmarks for documents and the web

[Matthew Dawson]

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

Review request for KDE Frameworks, Localization and Translation (l10n) and 
Matthew Dawson.


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


Repository: kconfig


Description
---

As requested in bug #1164383, this adds a new standard shortcut
for adding bookmarks for web pages.

BUG: 1164383

I'm not sure if this is the best approach.  If this is accpted, I'll commit a 
similar change into KConfigWidgets.


Diffs
-

  src/gui/kstandardshortcut.h 5bb07fb9f060b0d5950aed251da985ce3aa46661 
  src/gui/kstandardshortcut.cpp 84007374c2836c1d61cb9b9361bd4217aa4ddc32 

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


Testing
---

Compiles, verified context extracted properly into pot file.


Thanks,

Matthew Dawson

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

Re: Review Request 121372: Differeniate between bookmarks for documents and the web

[Matthew Dawson]

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

(Updated Dec. 6, 2014, 3:49 p.m.)


Review request for KDE Frameworks, Localization and Translation (l10n) and 
Matthew Dawson.


Changes
---

Copy+paste error on bug number.


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


Repository: kconfig


Description
---

As requested in bug #1164383, this adds a new standard shortcut
for adding bookmarks for web pages.

BUG: 1164383

I'm not sure if this is the best approach.  If this is accpted, I'll commit a 
similar change into KConfigWidgets.


Diffs
-

  src/gui/kstandardshortcut.h 5bb07fb9f060b0d5950aed251da985ce3aa46661 
  src/gui/kstandardshortcut.cpp 84007374c2836c1d61cb9b9361bd4217aa4ddc32 

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


Testing
---

Compiles, verified context extracted properly into pot file.


Thanks,

Matthew Dawson

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

Re: FAM in kdirwatch

[Matthew Dawson]

On November 28, 2014 01:16:02 PM Jonathan Riddell wrote:
 At Kubuntu we're trying to work out if we want kcoreaddons to build-dep on
 FAM.
 
 FAM isn't used if inotify is available and kdelibs never built against FAM.
 
 But we had this bug report about FAM being needed for NFS
  https://bugs.launchpad.net/ubuntu/+source/kde4libs/+bug/525005
 will build-dep on FAM fix uses using NFS and are there any downsides to
 building with FAM?
Be careful with FAM on local file systems if you use the old FAM daemon.  
There is a bug in either FAM or the kernel that breaks FAM's use of dnotify.  
Gamin doesn't suffer this issue on local disks (as it uses inotify).  I don't 
know if this would affect NFS users in either program.

I've been meaning to dig deeper and find out what is going on, I just haven't 
had time.
-- 
Matthew

smime.p7s
Description: S/MIME cryptographic signature
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 121272: Fix link to KSharedConfig::openConfig in apidoc

[Matthew Dawson]

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

Ship it!


Thanks!  Will push shortly.

- Matthew Dawson


On Nov. 27, 2014, 8:50 a.m., Heiko Becker wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121272/
 ---
 
 (Updated Nov. 27, 2014, 8:50 a.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 KSharedConfig has been ported to QStandardPaths in KF5.
 
 I don't have commit access, so I would need someone to push this.
 
 
 Diffs
 -
 
   src/core/kconfig.h cc16792 
 
 Diff: https://git.reviewboard.kde.org/r/121272/diff/
 
 
 Testing
 ---
 
 Generated the api docs and tested the link took me to KSharedConfig's page.
 
 
 Thanks,
 
 Heiko Becker
 


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

Re: Review Request 120283: make KConfigLoader more predictable by exposing a getter to KConfig's OpenFlags

[Matthew Dawson]

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

Ship it!


Ship It!

- Matthew Dawson


On Sept. 19, 2014, 7:51 a.m., Aaron J. Seigo wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120283/
 ---
 
 (Updated Sept. 19, 2014, 7:51 a.m.)
 
 
 Review request for KDE Frameworks and Matthew Dawson.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 When opening a KSharedConfig, the OpenFlags must match to share the 
 underlying config object. Otherwise a new KConfig is created and the benefit 
 of using KSharedConfig is lost. This was triggering subtle bugs with the use 
 of KConfigLoader in plasmashell.
 
 This patchset adddress that issue by exposing the OpenFlags used to open a 
 KConfig.
 
 Another approach would be to add another KSharedConfig::openConfig() which 
 takes a KConfig object and uses that in its search for a matching pointer 
 object, and then use that version of openConfig in KSharedConfig
 
 
 Diffs
 -
 
   src/core/kconfig.h d7d4b7df8f33e33a389527d47d02fc844b74aed3 
   src/core/kconfig.cpp c6dec43641611bf14c5eb04d3007dd9016bcb3c8 
   src/gui/kconfigloader.cpp 52ac6d17b427a6eb8519699f642759b7ae672b81 
 
 Diff: https://git.reviewboard.kde.org/r/120283/diff/
 
 
 Testing
 ---
 
 Used KConfigLoader in startup config of plasmashell and settings are now 
 propagated correctly from the shell scripting to the config loader objects.
 
 
 Thanks,
 
 Aaron J. Seigo
 


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

Re: Non-coinstallable frameworks

[Matthew Dawson]

On September 18, 2014 01:15:51 PM Aleix Pol wrote:
 Hi Ivan,
 I've just learned that the KActivities framework is not co-installable with
 kdelibs4.
 
Are you sure about this?  I just double checked, and my distribution (Gentoo) 
has both installed right now, in the same prefix.  And I'm running the latest 
git for kactivities/5, and a very recent kdelibs.

I know the runtime components can't be co-installed, but as far as I know 
kactivities/5's components are backwards compatible with kactivities/4.  If 
they are not, would that not be a bug that should be fixed?
-- 
Matthew

smime.p7s
Description: S/MIME cryptographic signature
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 119991: Use Juvia to add comments on the API class pages

[Matthew Dawson]

On September 12, 2014 03:19:13 AM Ben Cooksley wrote:
  On Sept. 8, 2014, 10:22 p.m., Ben Cooksley wrote:
  Matthew Dawson wrote:
  I don't think upstream would be happy with the patch, as it hijacks
  the moderator emails (which should link to the admin page) to send
  out notifications of when a comment occurs.  There was a suggestion
  on how to change the link in the email in a comment above (not a
  formal patch), which we could apply locally.  If syadmins don't want
  to carry the change locally (which I understand completely), then we
  should see about submitting such a feature upstream.
  
  Regardless of whether the change is kept locally, I agree it can be
  committed.  I'll throw the ship it switch, since it seems we are all
  in agreement.
 Hm, indeed. If someone could prepare such a patch that would be much
 appreciated. Perhaps it is necessary for us to add such functionality?
 
 
 - Ben
Hi all,

I sat down and learnt some ruby, and came up with this: 
https://github.com/phusion/juvia/pull/63 (branch kde_addition in repository 
https://github.com/MJDSys/juvia.git ).  This is a more complete patch based 
upon the hint from Denis, but I tried to design it to be more useful for 
api.kde.org (so it hides user email addresses, for instance).

Let me know if it works for KDE (it seems to work locally on my laptop using 
the test email system in RoR), or if any modifications are necessary.
-- 
Matthew

smime.p7s
Description: S/MIME cryptographic signature
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 119991: Use Juvia to add comments on the API class pages

[Matthew Dawson]

 On Sept. 8, 2014, 6:22 p.m., Ben Cooksley wrote:
  What is the status of this? Is there anything blocking it being shipped and 
  made available on api.kde.org?
 
 Denis Steckelmacher wrote:
 It works locally, so I'm just waiting for a ship-it or other comments :-)
 
 Aleix Pol Gonzalez wrote:
 Let's see if we can get this discussed in the KF5 BoF today.
 
 Aleix Pol Gonzalez wrote:
 I kind of forgot... :(
 
 Anyway, +1 for me. If there are no reasons to keep postponing it, I'd say 
 let's just do this.

The only issue I see is that the email notifications has the admin url 
included, which it appears I cannot access, making it hard to find the new 
comment.  There was a suggested change to juvia, which would fix this.  Has 
that been applied?

Otherwise, +1 for me too.


- Matthew


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


On Aug. 29, 2014, 10:16 a.m., Denis Steckelmacher wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/119991/
 ---
 
 (Updated Aug. 29, 2014, 10:16 a.m.)
 
 
 Review request for KDE Frameworks and Aurélien Gâteau.
 
 
 Repository: kapidox
 
 
 Description
 ---
 
 Juvia is a Free Software commenting system that can easily be used on static 
 websites like api.kde.org. An instance of Juvia has just been installed on 
 commenting.kde.org (many thanks to Ben Cooksley!), and this patch adds 
 support for it to api.kde.org.
 
 The users can now comment class pages. The comments are disabled (no comment 
 box nor anything else appears) on the main page of each framework, on the 
 Frameworks 5 index page and on any other page that does not directly concern 
 a class. I've done that in order to avoid cluttering important pages with 
 comments, but if you think that having comments on all the pages (or a bigger 
 subset of them) is desirable, it is very easy to change. Personally, I would 
 avoid having comments on the main pages, so that any spam, if the automatic 
 Akismet filter does not work, will not be too visible.
 
 A screenshot is linked to this review request and shows how the comments have 
 been integrated (I slightly modified the built-in Juvia style so that Doxygen 
 and KDE colors are used).
 
 
 Diffs
 -
 
   src/kapidox/data/htmlresource/kde.css e173dfe 
   src/kapidox/data/templates/comments.html PRE-CREATION 
   src/kapidox/data/templates/doxygen.html d00e14e 
 
 Diff: https://git.reviewboard.kde.org/r/119991/diff/
 
 
 Testing
 ---
 
 Posting a comment works, and comments can be viewed. They also appear in the 
 admin interface of Juvia (I've now deleted these comments). I posted comments 
 on different classes and in different frameworks in order to test that 
 namespacing works correctly.
 
 
 File Attachments
 
 
 Comments on api.kde.org
   
 https://git.reviewboard.kde.org/media/uploaded/files/2014/08/29/278299d8-18a6-46a0-ada5-1b6452a3276f__apidox-comments-1.png
 
 
 Thanks,
 
 Denis Steckelmacher
 


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