Re: KOpeningHours in kdereview

[Rolf Eike Beer]

> For the flex/bison code it might be possible to factor out these parts, as
> it's largely very basic C++ code anyway. You'd need to rebuild most of the
> logic around that though I'm afraid.

Everything that works and can help me spot syntax errors help. If I can't 
parse some things then it is not worse than now where it it just an opaque 
string.

Eike

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

Re: KOpeningHours in kdereview

[Rolf Eike Beer]

> but meanwhile it's also being
> evaluated for use in OSM validation tooling:
> https://github.com/osm-fr/osmose-backend/issues/555. That has already
> resulted in a number of contributions increasing the tolerance for
> non-conforming expressions, which benefits all other use-cases as well.

I was thinking about doing something similar for OSM2go, which currently also 
runs on the N900 and needs gcc 4.2 for it. Is there interest to have a C++98 
STL only version of the main parser? Or is it so tied to Qt classes that there 
is no chance in that?

Eike

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

Re: KOSMIndoorMap in kdereview

[Rolf Eike Beer]

Am Donnerstag, 22. Oktober 2020, 17:25:32 CEST schrieb Volker Krause:
> Hi,
> 
> KOSMIndoorMap is a QML component for showing multi-floor OSM indoor maps (as
> its very creative name might suggest). It's using maps.kde.org as a data
> source (same as Marble), and has been created to show interactive maps of
> train stations for KDE Itinerary.
> 
> https://invent.kde.org/libraries/kosmindoormap
> 
> KOSMIndoorMap has been growing inside the KPublicTransport repo (due to some
> building blocks being available there alreay and me being lazy), and has
> now been split into its own repo. So technically this is coming out of a
> reviewed repo already rather than from playground, but better be on the
> safe side :)
> 
> The aim is having this (together with KPublicTransport and KDE Itinerary)
> join the release service for 20.12.

The const version of DataSet::way() returns Way*, but the non-const version 
returns OSM::Way*. I guess the extra qualification is not needed.

In OSM2go I use std::unordered_map to store the different types of objects 
which makes lookups much easier. YMMV depending if you want to optimize for 
lookup or memory size.

My recent work there is to get rid of the note, way, and relation prefixes or 
suffixes on function names and make a template from the remaining 
implementation so I don't have to implement the same things 3 times.

I have derived all 3 object types from a common base class, which allows me to 
simplify things like Element::url() by just calling obj->url() and let a 
virtual overload do the rest. Which is a good example: the final return in 
that function should be replaced by Q_UNREACHABLE(), too.

The nodes vectors in Element::outerPath() could benefit from a call to 
reserve().

The coding style for the remark-if in XmlParser::parse() is inconsistent. Or, 
if compared to the rest of the file, it's the only consistent one in that 
function.

Eike

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

Re: Information regarding upcoming Gitlab Migration

[Rolf Eike Beer]

Bhushan Shah wrote:


I've worked on draft "move" of the current set of the repositories in
their respective subgroups at the repo-metadata project's branch [1].
You can browse the directory structure to get idea of how final
structure on Gitlab would look like.


No objection, just a request for clarification: it looks like everything 
in extragear or playground was merged into there, right?


Eike

Re: Installing qml caches

[Rolf Eike Beer]

Am 2018-06-01 13:10, schrieb Alexander Volkov:

Hi all,

It would be nice to install .qmlc files in addition to .qml files to
reduce start-up time of applications.
They are generated with qmlcachegen. For Qt 5.11:
qmlcachegen -o example.qmlc qxample.qml

Currently qml files are usually installed the following way:
install(DIRECTORY qml/ DESTINATION ${KDE_INSTALL_QMLDIR}/org/kde/kcm)

So this could be changed to something like
ecm_install_qml(qml org/kde/kcm)

I wonder whether someone already working on this or want to implement
such a function?
If not, I'll try to do it myself.


Just a quick note: the quick compiler macros provided by Qt 5.11.0 are 
broken for cross-compilation (QTBUG-68724).


Eike

Re: Symmy in kde-review

[Rolf Eike Beer]

Am Montag, 4. Dezember 2017, 22:14:14 schrieb Elvis Angelaccio:
> On lunedì 4 dicembre 2017 10:25:48 CET, Tomaz Canabrava wrote:
> > On Sun, Dec 3, 2017 at 11:14 PM, Albert Astals Cid  wrote:
> > El dijous, 23 de novembre de 2017, a les 10:34:41 CET, Elvis Angelaccio va
> > 
> > escriure:
> >> Hi,
> >> symmy has been moved to kde-review for the usual review process.
> >> 
> >> It's a tiny frontend for the symmetric encryption functionality of GPG.
> >> It
> >> doesn't handle signing or public/private keys, as we already have kgpg or
> >> kleopatra for that.
> > 
> > How hard would be to make that functionality to kgpg (simple
> > encryption without public / private keys) instead of yet -
> > another tool to handle file encryption?
> 
> Not sure, perhaps Eike can better answer that (kgpg already does symmetric
> encryption).
> I chose to go with an external process (+ qgpgme) for technical reasons
> (basically, to not freeze the dolphin UI). Since this means we get a
> self-contained binary, it can as well go in its own repo imho.

Right click on a file, Actions, Encrypt File, click Options, chose "symmetric 
encryption". All in a different process, so Dolphin is not blocked. If it's 
really an issue adding an own option to trigger this through their own action 
from the context menu.

Eike

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

Re: Review Request 130193: [cmake]: De-duplicate "else" to fix build with cmake-3.9

[Rolf Eike Beer]

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


Ship it!




While at it, please also remove the arguments from the else() and endif() calls.

- Rolf Eike Beer


On Juli 20, 2017, 7:59 nachm., Heiko Becker wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130193/
> ---
> 
> (Updated Juli 20, 2017, 7:59 nachm.)
> 
> 
> Review request for kdelibs and Frank Osterfeld.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Otherwise it errors out with:
> "CMake Error at kdeui/CMakeLists.txt:316 (else): A duplicate ELSE
> command was found inside an IF block."
> Also adjust indentation to match the surrounding lines.
> 
> 
> Diffs
> -
> 
>   kdeui/CMakeLists.txt d6ec8b47e9af686441ab5ab761be9ff424cbb556 
> 
> Diff: https://git.reviewboard.kde.org/r/130193/diff/
> 
> 
> Testing
> ---
> 
> Builds fine with cmake-3.9.0.
> 
> 
> Thanks,
> 
> Heiko Becker
> 
>

Re: kdereview: ksmtp

[Rolf Eike Beer]

Am Donnerstag, 11. Mai 2017, 17:03:01 schrieb Daniel Vrátil:
> Hi,
> 
> please review ksmtp, which is now in kdereview.

-the CMakeLists.txt has a mix of spaces inside () or not

-in loginjob, line 173, you check for code 25. Should this be 250? Or is that 
25*? Where is ServerResponse actually defined, I only see the header.

-does that support pipelining? I don't see any sync points, so I guess not.

-there is a longstanding bug in KMail that it violates the RfC when it has a 
problem with authentication (e.g. password rejected), that is does not 
properly QUIT the SMTP session, but just closes the socket. Is that properly 
handled?

Greetings,

Eike

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

Re: Review Request 128941: ZLIB dependency is in libkonq since 7635179

[Rolf Eike Beer]

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




konqueror/src/CMakeLists.txt 
<https://git.reviewboard.kde.org/r/128941/#comment66843>

If Konqueror directly uses these symbols it needs to explicitely link to 
it, even if libkonq does.


- Rolf Eike Beer


On Sept. 18, 2016, 11:36 nachm., Andreas Sturmlechner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128941/
> ---
> 
> (Updated Sept. 18, 2016, 11:36 nachm.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> ---
> 
> Add ECMMarkAsTest, used by fsview tests
> 
> 
> Diffs
> -
> 
>   konqueror/CMakeLists.txt 53c4829cbc2f8b380ad8608b555eb6e15b24a3bb 
>   konqueror/src/CMakeLists.txt e8e408611335fa56faf24466307f83e40a3b70ee 
>   lib/konq/CMakeLists.txt 7a61493ff13561340c1a6c114763489343212f41 
> 
> Diff: https://git.reviewboard.kde.org/r/128941/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andreas Sturmlechner
> 
>

Re: kconfig_compiler help ( reproducible builds )

[Rolf Eike Beer]

Am Freitag, 27. Mai 2016, 09:18:25 schrieb Scarlett Clark:
> I still need help with this, hacking the packaging was not accepted.
> Here is the exact problem:
> https://reproducible-builds.org/docs/locales/#collation-order
> 
> Looking at kconfig_compiler code that only thing that is standing out is:
> 
> https://github.com/KDE/kdelibs/blob/23f0972e03d9d5f30412b7c9c74cb4cadef7293a
> /kdecore/kconfig_compiler/kconfig_compiler.cpp#L481
> 
> 
> Am I on the right track? If so can someone with more C++ knowledge help me
> with a solution?

Is this actually outputting UTF8 sequences? I would expect a call to .toUtf8() 
somewhere in there.

Eike

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

Re: Review Request 104960: KJS: fix behaviour on allocation errors

[Rolf Eike Beer]

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

(Updated Feb. 7, 2016, 5:29 nachm.)


Status
--

This change has been discarded.


Review request for kdelibs.


Repository: kdelibs


Description
---

The KJS allocator will likely crash with a 0-deref on allocation errors. The 
exact behaviour will also depend on the platform, e.g. a Un*x platform without 
posix_memalign() will have MAP_FAILED as the pointer used for calculations 
(which is (void*)-1), other will have 0.

This will make the allocator have a sane default behaviour: just return 0.


Diffs
-

  kjs/collector.cpp 70e4757 

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


Testing
---


Thanks,

Rolf Eike Beer

Re: Change to Mail Infrastructure - SPF and DKIM verification will now be enforced

[Rolf Eike Beer]

Am 04.12.2015 11:08, schrieb Ben Cooksley:
On Fri, Dec 4, 2015 at 9:01 AM, Rolf Eike Beer 
<k...@opensource.sf-tec.de> wrote:


Think of SPF: I sent an email to a kde.org email address only some 
weeks ago.
My domain sets a SPF policy. The KDE server accepts this (it's 
actually
correct), and then sends the mail on (unaltered). Now the next server 
also
checks SPF and will reject the mail, because the KDE server is not 
allowed to
send mail for my domain. Now you have 2 ways out: either the KDE 
server
rewrites the "mail from" header (what you will later find as 
Return-Path in the
mail header), or the final destination says allows the user to say 
"hey, I use
those kde.org server as a forwarder to me, so whatever SPF says, mails 
from
that host are fine". Both ways work, both are fine, but both require 
some sort

of action somewhere on the path.


Rewriting to workaround SPF restriction is also standardised - as a
mechanism known as SRS - see http://www.openspf.org/SRS


Has KDE implemented this in the last few weeks? Before it was not.


That part is simple. For DKIM stuff get's more complicated because you
sometimes _have_ to modify the body, e.g. when you need to base64- or 
qp-
recode parts of the mail because the receiving mail server does not 
support
8bit-transfer (which is an issue by itself, but still sadly legal). So 
with
DKIM you are actually screwed at this point. The only good way it is 
again to
permit your users to ignore DKIM signatures from certain hosts (e.g. 
if you
subscribe to a Debian list, then simply ignore DKIM for the Debian 
servers).

Finding out those itself is not an easy task either.

So all in all one can enable DKIM for list services, but for user 
accounts it
should be opt-in with an easy way to whitelist certain hosts for 
relaying.

Everything else is just asking for endless bounces.


Note that DKIM senders and receivers are usually running on modern
infrastructures, so 8bit transfer shouldn't be an issue.
For user to user transmission, there is no reason why mail bodies
would be modified.


Well, nice try. Out of 5 mail providers I checked 3 failed: AOL, GMX.de, 
Web.de.


Greetings,

Eike

Re: Change to Mail Infrastructure - SPF and DKIM verification will now be enforced

[Rolf Eike Beer]

Ben Cooksley wrote:
> On Fri, Dec 4, 2015 at 11:28 PM, Jan Kundrát  wrote:
> > On Friday, 4 December 2015 10:56:42 CET, Ben Cooksley wrote:
> >> Note that in the long run with DMARC looming you will need to switch
> >> to #2 anyway, and keeping your current behaviour will likely lead to
> >> mail from people who use Yahoo / AOL / etc ending up in the spam
> >> folder with many mailing list members. I'll be starting a discussion
> >> regarding taking this step on KDE systems at some point in the near
> >> future (switching to DMARC compatible policies).
> >> 
> >> For more information, please see http://wiki.list.org/DEV/DMARC
> > 
> > Do I understand your plan correctly? The following projects appear to not
> > re-sign their ML traffic, and they mangle headers at the same time. If I
> > understand your plan correctly, this means that I won't be able to use my
> > @kde.org addresses on mailing lists of these projects, for example:
> > 
> > - Qt,
> > - Debian,
> > - Gentoo,
> > - OpenStack,
> > - anything hosted at SourceForge,
> > - and many, many more, essentially anybody who were ignoring DKIM.
> > 
> > Please, change your plans, this is obviously a huge no-go.
> 
> *Sigh*.
> 
> Debian has already committed (prior to any of this) to making their
> mailing lists DMARC compliant by ceasing the alteration of mail
> passing through their lists.

Which is a good idea anyway, as far as you can influence it (see the 8bit 
problems from the other mail).

> It is an extreme pity these mailing list providers have demonstrated
> such an extreme disregard for standards which aim to eliminate
> forgeries and ensure people cannot be digitally misrepresented. This
> is why we had to change Bugzilla a while back to send as
> bugzilla_nore...@kde.org instead of the acting user's email address -
> because Yahoo's enforcement policy meant GMail always placed mail from
> Yahoo users in the spam folder.

Huh? Of course you _must_ send with a @kde.org address. My SPF policy forbids 
you to send mail for my domain. And now you want to enforce SPF, but don't 
properly handle it yourself?

> I'll grant an extension until 31 January, however I would like to see
> commitments from some of these to improve their infrastructure.

It wont affect me, as I ignore the whole DKIM stuff both at the sending and 
receiving end, but this just going to cause a lot of unnecessary trouble I 
think.

To make it clear: I receive tons of spam per day. It has become worse in the 
last month, as it seems that the usual filters do not work that good anymore. 
You as postmaster of such a public domain are likely receiving even more of 
that crap. But that proposal is going to cause collateral damage.

Eike

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

Re: Change to Mail Infrastructure - SPF and DKIM verification will now be enforced

[Rolf Eike Beer]

Am Donnerstag, 3. Dezember 2015, 11:54:43 schrieb Jan Kundrát:
> On Thursday, 3 December 2015 07:13:07 CET, Ben Cooksley wrote:
> > I will be re-enabling DKIM validation in one week's time - which will
> > then break subscriptions to Debian mailing lists (as any email from
> > anyone who has enabled DKIM which hits their lists will not be
> > accepted by KDE email infrastructure)
> 
> Ben, could you please briefly explain your idea about how a complying
> mailing list service should behave? Suppose that I have an installation of
> mlmmj which:
> 
> - mangles the Subject header,
> - preserves the original From header,
> - maybe replaces a Reply-To with the ML's address,
> - introduces a bunch of specific List-* headers,
> - otherwsie doesn't manipulate the MIME tree or the message texts.
> 
> What should I do to make sure that this service continues working once you
> flip the switch?

This is actually a flaw in those standards: they think that everyone you 
communicate with will comply. If you as a mailing list service do not then 
everyone that sends to your list will eventually get in trouble if you do not 
rewrite the message or resigns it.

Think of SPF: I sent an email to a kde.org email address only some weeks ago. 
My domain sets a SPF policy. The KDE server accepts this (it's actually 
correct), and then sends the mail on (unaltered). Now the next server also 
checks SPF and will reject the mail, because the KDE server is not allowed to 
send mail for my domain. Now you have 2 ways out: either the KDE server 
rewrites the "mail from" header (what you will later find as Return-Path in the 
mail header), or the final destination says allows the user to say "hey, I use 
those kde.org server as a forwarder to me, so whatever SPF says, mails from 
that host are fine". Both ways work, both are fine, but both require some sort 
of action somewhere on the path.

That part is simple. For DKIM stuff get's more complicated because you 
sometimes _have_ to modify the body, e.g. when you need to base64- or qp-
recode parts of the mail because the receiving mail server does not support 
8bit-transfer (which is an issue by itself, but still sadly legal). So with 
DKIM you are actually screwed at this point. The only good way it is again to 
permit your users to ignore DKIM signatures from certain hosts (e.g. if you 
subscribe to a Debian list, then simply ignore DKIM for the Debian servers). 
Finding out those itself is not an easy task either.

So all in all one can enable DKIM for list services, but for user accounts it 
should be opt-in with an easy way to whitelist certain hosts for relaying. 
Everything else is just asking for endless bounces.

> I would like to have more information about what you mean by "DKIM
> validation" -- what specific steps are you going to introduce, and how is
> the end result going to react to a missing or invalid DKIM signatures.
> 
> Also, quoting RFC 6376, section 6.3:
> 
>In general, modules that consume DKIM verification output SHOULD NOT
>determine message acceptability based solely on a lack of any
>signature or on an unverifiable signature; such rejection would cause
>severe interoperability problems.  If an MTA does wish to reject such
>messages during an SMTP session (for example, when communicating with
>a peer who, by prior agreement, agrees to only send signed messages),
>and a signature is missing or does not verify, the handling MTA
>SHOULD use a 550/5.7.x reply code.
> 
> That seems in line with what e.g. GMail is doing, only enforcing DKIM
> validation for notoriously faked domains like eBay and PayPal where the
> phishing potential is high.

No, this means that a mail should not be rejected if there is _no_ signature, 
or a malformed one. If a domain publishs that it does DKIM signing and mails 
are expected to be correctly signed, then rejecting on a invalid signature is 
actually fine (and the sole purpose of this RfC).

Greetings,

Eike

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

Re: Review Request 124824: [OS X] FindKDE4Internal.cmake : reintroduce a cmake_minimum_required statement

[Rolf Eike Beer]

Am Mittwoch, 19. August 2015, 21:14:38 schrieb Heiko Becker:
 I stumbled upon the same, it's actually a bug in cmake fixed by this commit:
 
 http://www.cmake.org/gitweb?p=cmake.git;a=commit;h=b9ec9392da21a3421e48c6961
 976060d872faffb

But the short fix for this is indeed to put cmake_minimum_required() into your 
project. But not into a project you include, but into the main project you are 
trying to build.

Greetings,

Eike

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

Re: Review Request 122987: Allow user to specify path to myspell dictionary files

[Rolf Eike Beer]

 On März 17, 2015, 1:07 nachm., Laurent Montel wrote:
  src/plugins/hunspell/hunspellclient.cpp, line 27
  https://git.reviewboard.kde.org/r/122987/diff/3/?file=355372#file355372line27
 
  #include ...
  we use local file.

No, the file is in an include path, not in the same directory as the source 
file (binary vs. source dir).


- Rolf Eike


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


On März 17, 2015, 1:09 nachm., Eugene Shalygin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122987/
 ---
 
 (Updated März 17, 2015, 1:09 nachm.)
 
 
 Review request for KDE Frameworks and kdelibs.
 
 
 Repository: sonnet
 
 
 Description
 ---
 
 Not on all systems myspell dictionaries are located in 
 /usr/share/myspell/dicts/,  which is hardcoded in the hunspell plugin 
 sources. For instance, on Gentoo it is /usr/share/myspell/. So let the user 
 define this path.
 
 
 Diffs
 -
 
   src/plugins/hunspell/CMakeLists.txt 098fb49 
   src/plugins/hunspell/config-hunspellplugin.h.cmake PRE-CREATION 
   src/plugins/hunspell/hunspellclient.cpp 9e3aa67 
   src/plugins/hunspell/hunspelldict.cpp 621113d 
 
 Diff: https://git.reviewboard.kde.org/r/122987/diff/
 
 
 Testing
 ---
 
 hunspell plugin began to work on Gentoo
 
 
 Thanks,
 
 Eugene Shalygin
 

Re: KDE4 SSL

[Rolf Eike Beer]

Am Dienstag 21 Oktober 2014, 10:52:38 schrieb Thiago Macieira:
 On Monday 20 October 2014 21:00:51 Pali Rohár wrote:
  Hello!
  
  Do you know which KDE4 libraries are using SSL and TLS protocols?
  And it is now possible to disable SSLv2 and SSLv3 protocols in
  KDE4 libraries?
 
 That would be QtNetwork, by way of kdecore and kio. SSLv2 is already
 disabled by default. To disable SSLv3 by default, you need to modify
 QtNetwork.

What happens if OpenSSL is built with no-ssl3?

Greetings,

Eike

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

Re: Review Request 120543: Update FindPostgreSQL.cmake

[Rolf Eike Beer]

Am 10.10.2014 08:46, schrieb Jaroslaw Staniek:
On 10 October 2014 08:05, Rolf Eike Beer k...@opensource.sf-tec.de 
wrote:
Update FindPostgreSQL.cmake to make is useful. Based on cmake's (3.x) 
one
but further improved PostgreSQL_TYPE_INCLUDE_DIR lookup. The fix 
comes from

libpredicate (master).


I see no upstream bug report for this.


Would a bug report for Calligra master be OK for you?
This is the only user of the PostgreSQL_TYPE_INCLUDE_DIR in entire KDE
I the know about:

http://lxr.kde.org/search?_filestring=_string=PostgreSQL_INCLUDE_DIR


You are not looking for PostgreSQL_TYPE_INCLUDE_DIR here, but for 
PostgreSQL_INCLUDE_DIR. And there seems to be no user of that at all 
inside KDE.



I am sorry if I misunderstood.

Good thing that the file disappears in KF5, since cmake has pretty
good own copy (not sufficient but I'll try to patch in the upstream).


CMake is the relevant upstream, Calligra is downstream (i.e. it uses our 
stuff).


Eike

Re: Review Request 120202: [OS X] improvements to the kwallet/OSX keychain integration

[Rolf Eike Beer]

 OK, implementation question.
 
 How do I declare a slot in a private class that doesn't have a specific
 header file? Putting `private QSLOT` above the function definition makes
 things compile, but the runtime complains about a missing slot (curiously
 even expecting it in KWallet::Wallet). Yes, I did update the connect call
 to pass in the pointer to the parent class 

Use Q_PRIVATE_SLOT in the public header?

Eike

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

Re: Review Request 120287: [OS X] make kde-workspace build

[Rolf Eike Beer]

Am Freitag, 19. September 2014, 22:57:40 schrieb René J.V. Bertin:
  On Sept. 20, 2014, 12:26 a.m., Christoph Feck wrote:
   You added APPLE to the if() but not always to the matching endif()...
 
 True. But that's optional, no?

The endif needs to have either a matching argument to the last if or elseif, 
or an entirely empty argument.

Eike

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

Re: Review Request 119498: Report crashes of KDE apps in Apple OS X (2)

[Rolf Eike Beer]

 In this review we have three portability problems:
 
 1. On Apple OS X, Dr Konqi's dialog box hides itself underneath the main
 window of the app that has just crashed, so is effectively useless. This
 appears to be because Dr Konqi is started by a Linux/Unix method (fork() +
 exec()?). If an app is started with the Apple OS X open command, it
 always appears on top. The patch just raises the dialog box.

Maybe that should be unconditionally done, who knows what the next fancy Linux 
DE would do.

 2. When formatting the backtrace output, Dr Konqi crashes (with an ASSERT)
 on the last line. This appears to be an error in the algorithm used (i.e.
 also a bug in Linux KDE), but the patch is treating it as an Apple OS X
 portability problem for now.

From staring at the code I suspect that this has something to do with 
additional linebreaks in the lines. From what I see the input lines may also 
contain linebreaks, this is why the internal line map has holes in the key 
sequence. I suspect the crash you see is somehow related to if the last line 
itself has \n in it or not or something like that. Looks like this needs a 
unittest ;)

Greetings,

Eike

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

Re: Review Request 116097: No longer use strlcpy in kstartupconfig

[Rolf Eike Beer]

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


When you start poking at this code, why not kill the entire fopen/fgets/fixed 
size buffer stuff and replace it with std streams or something like that?

- Rolf Eike Beer


On Feb. 26, 2014, 6:11 p.m., Alexander Richardson wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/116097/
 ---
 
 (Updated Feb. 26, 2014, 6:11 p.m.)
 
 
 Review request for kde-workspace.
 
 
 Repository: kde-workspace
 
 
 Description
 ---
 
 No longer use strlcpy in kstartupconfig
 
 This means we no longer need to find libbsd on Linux.
 kstartupconfig is now uses std::string instead of pure C strings, but
 the performance difference should be minimal (and it is less error-prone).
 
 
 Diffs
 -
 
   CMakeLists.txt bce3cd1de65b8420a859c342db981919965071ba 
   cmake/modules/FindLibBSD.cmake 6383083023cdbfbeffcab0cef98f48102e593d38 
   kstartupconfig/CMakeLists.txt ceebd6a65af4cdaa717cfb0dcff5097121cf48d9 
   kstartupconfig/kstartupconfig.c d8e65f48a75dad49fe6c585f89260c2a6d483809 
 
 Diff: https://git.reviewboard.kde.org/r/116097/diff/
 
 
 Testing
 ---
 
 compiles
 
 
 Thanks,
 
 Alexander Richardson
 

Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests

[Rolf Eike Beer]

Am 12.02.2014 10:56, schrieb Dawit Alemayehu:

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

(Updated Feb. 12, 2014, 9:56 a.m.)


Review request for kdelibs and Andrea Iacovitti.


Changes
---

Uploaded the patch.


It looks to me as if the patch also contains the stuff for RR 115651.

Eike

Re: Review Request 115651: Fix HTTP redirection handling (3XX status code)

[Rolf Eike Beer]

Am Mittwoch, 12. Februar 2014, 17:10:49 schrieb Andrea Iacovitti:
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/115651/#review49669
 ---
 
 
 As stated in the bug report it is also true that every other browsers
 rewrite POST method to GET when following 301/302 redirections. This
 behavior could also be verified in curl by issuing the following commands:
 curl -L --data fakepostdata
 http://greenbytes.de/tech/tc/httpredirects/redirect_with_status.cgi?301
 curl -L --data fakepostdata
 http://greenbytes.de/tech/tc/httpredirects/redirect_with_status.cgi?302 We
 could/should do the same for compatibility.
 In that case the snippet of code that handles 301-303 http status codes may
 assume this form:
 
 } else if (m_request.responseCode = 301  m_request.responseCode=
 303) { // NOTE: This is wrong according to RFC 2616 (section 10.3.[2-4,8]).
 // However, because almost all client implementations treat a 301/302 //
 response as a 303 response in violation of the spec, many servers // have
 simply adapted to this way of doing things! Thus, we are // forced to do
 the same thing. Otherwise, we loose compatibility and // might not be able
 to correctly retrieve sites that redirect. if (m_request.responseCode ==
 301) { // Moved permanently
 setMetaData(QLatin1String(permanent-redirect), QLatin1String(true)); if
 (m_request.method == HTTP_POST) {
 m_request.method = HTTP_GET; // FORCE a GET
 setMetaData(QLatin1String(redirect-to-get),
 QLatin1String(true)); }
 } else if (m_request.responseCode == 302) { // Moved temporarily
 if (m_request.method == HTTP_POST) {
 m_request.method = HTTP_GET; // FORCE a GET
 setMetaData(QLatin1String(redirect-to-get),
 QLatin1String(true)); }
 } else { // 303 See Other
 if (m_request.method != HTTP_HEAD) {
 m_request.method = HTTP_GET; // FORCE a GET
 setMetaData(QLatin1String(redirect-to-get),
 QLatin1String(true)); }
 }
 }
 
 ...or something like that.

switch () { … }

Eike

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

Re: Moving Baloo forward

[Rolf Eike Beer]

Am Dienstag, 28. Januar 2014, 20:10:13 schrieb Ivan Čukić:
  To move a file to another machine and have the metadata be copied and re-
  indexed on that new machine as well. The copy process just needs to take
  care of transfering the xattr. This can even work when using a USB stick
  or so as interim medium.
 
 I'm all for xattrs, but this thread really raises a nice question of which
 annotations/tags/whatever should be public and which should not.

IMHO the default should be privacy first. Probably everyone of us has laughed 
about someone who accidentially published either metadata or deleted text 
(remember MS Word document history or something)? I'm absolutely fine if you 
want this that you do this. But most people will not be aware of it, even less 
of the implications. So it should be deactivated by default and only activated 
explicitely.

Eike

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

Re: Moving Baloo forward

[Rolf Eike Beer]

Am Freitag, 24. Januar 2014, 15:59:12 schrieb Vadim Zhukov:
 2014/1/24 Sebastian Kügler se...@kde.org:
  On Friday, January 24, 2014 01:24:54 Vadim Zhukov wrote:
  in the best case you'll have two totally different codepaths
  that you'll have to manage.
  
  This should be worst case, I think. In the best case, there's xattr
  support, meaning another code path isn't needed.
 
 It's doubtful at least whether xattr support is a good thing, because
 it's too easily gets misused: for example, there were viruses on
 Windows which effectively hidden themselves in NTFS streams. And there
 are personal data leaking issues I've pointed out earlier. But the
 question xarttrs: to be or not to be everywhere is totally offtopic,
 because it's what OS developers should decide.

I am against storing metadata in xattrs, but more for personal opinion than 
for anything I can express in technical terms. What could be done and which 
uses xattrs for what I seem them for is to just store a unique identifier in 
the xattrs (e.g. a GUID) which makes it easier to map the file to its metadata 
in the database, maybe together with some sort of checksum to detect 
modifications. That way no accidential information leak will happen if that 
thing is copied elsewhere.

Eike

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

Re: Review Request 114737: KInfocenter/OpenGL: reimplement the ReadPipe() function with QProcess

[Rolf Eike Beer]

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



kinfocenter/Modules/opengl/opengl.cpp
https://git.reviewboard.kde.org/r/114737/#comment33389

This allocates a new object on the heap that is never freed. Just use 
QProcess pipe;


- Rolf Eike Beer


On Jan. 4, 2014, 9 a.m., Wolfgang Bauer wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/114737/
 ---
 
 (Updated Jan. 4, 2014, 9 a.m.)
 
 
 Review request for kde-workspace and David Stephen Hubner.
 
 
 Repository: kde-workspace
 
 
 Description
 ---
 
 This patch reimplements the ReadPipe() function by using QProcess instead of 
 popen().
 This should make it more portable.
 
 As a positive side-effect, this also removes those sh: lspci: command not 
 found. messages when run in Konsole and lspci is not in the user's path.
 
 This was suggested on the kde-core-devel mailinglist in November:
 http://lists.kde.org/?l=kde-core-develm=138407113011843w=2
 http://lists.kde.org/?l=kde-core-develm=138409755820003w=2
 
 
 Diffs
 -
 
   kinfocenter/Modules/opengl/opengl.cpp 8901957 
 
 Diff: https://git.reviewboard.kde.org/r/114737/diff/
 
 
 Testing
 ---
 
 Ran KInfocenter with lspci in /usr/bin/ (i.e. in the user's path) and /sbin/ 
 (not in the user's path). The OpenGL module showed the 3D accelerator info 
 correctly in both cases.
 With lspci removed completely it showed unknown as expected.
 
 
 Thanks,
 
 Wolfgang Bauer
 

Re: Review Request 114737: KInfocenter/OpenGL: reimplement the ReadPipe() function with QProcess

[Rolf Eike Beer]

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


Looks fine to me, although I have not tested it.

One minor nitpick (that doesn't deserve a new diff): you could make the 
FileName argument to that function const QString .

- Rolf Eike Beer


On Jan. 4, 2014, 4:54 p.m., Wolfgang Bauer wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/114737/
 ---
 
 (Updated Jan. 4, 2014, 4:54 p.m.)
 
 
 Review request for kde-workspace and David Stephen Hubner.
 
 
 Repository: kde-workspace
 
 
 Description
 ---
 
 This patch reimplements the ReadPipe() function by using QProcess instead of 
 popen().
 This should make it more portable.
 
 As a positive side-effect, this also removes those sh: lspci: command not 
 found. messages when run in Konsole and lspci is not in the user's path.
 
 This was suggested on the kde-core-devel mailinglist in November:
 http://lists.kde.org/?l=kde-core-develm=138407113011843w=2
 http://lists.kde.org/?l=kde-core-develm=138409755820003w=2
 
 
 Diffs
 -
 
   kinfocenter/Modules/opengl/opengl.cpp 8901957 
 
 Diff: https://git.reviewboard.kde.org/r/114737/diff/
 
 
 Testing
 ---
 
 Ran KInfocenter with lspci in /usr/bin/ (i.e. in the user's path) and /sbin/ 
 (not in the user's path). The OpenGL module showed the 3D accelerator info 
 correctly in both cases.
 With lspci removed completely it showed unknown as expected.
 
 
 Thanks,
 
 Wolfgang Bauer
 

Re: Review Request 113779: KInfocenter/OpenGL: fix ReadPipe() in the case that the command cannot be run

[Rolf Eike Beer]

 ReadPipe() doesn't return 0 as expected in the case that the command is not
 found. but the length of sh's output which is command not found in this
 case. This is because popen() does not fail if the command is not found,
 because it _can_ run sh. (according to the man page, popen calls /bin/sh
 -c command) To fix this, ReadPipe() should check the return code of the
 call to pclose() (see man pclose), and return 0 if this is not equal to
 0.

Can't this be ported to simply use QProcess instead?

Eike

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

Re: Review Request 112528: Make it possible to preserve mtime for copy jobs not just move ones

[Rolf Eike Beer]

 Diffs
 -
 
   kio/kio/job.cpp 8ff088c
 
 Diff: http://git.reviewboard.kde.org/r/112528/diff/

Is it reviewboard fooling me or is there no diff?

Eike

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

Re: Review Request 112528: Make it possible to preserve mtime for copy jobs not just move ones

[Rolf Eike Beer]

Am Donnerstag 05 September 2013, 08:43:48 schrieb Rolf Eike Beer:
  Diffs
  -
  
kio/kio/job.cpp 8ff088c
  
  Diff: http://git.reviewboard.kde.org/r/112528/diff/
 
 Is it reviewboard fooling me or is there no diff?

This one does exist, I sent the mail for the wrong RR:

http://git.reviewboard.kde.org/r/112529/diff/

This does not exist.

Eike

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

Re: KDirWatch bug and the analysis. Help is welcome!

[Rolf Eike Beer]

David Faure wrote:
 On Thursday 01 August 2013 01:30:08 Mark wrote:
  However, we have been given the power of inotify which gives more detailed
  signals and lets us know which files have been created/added/modified
  which
  we should be used imho.
 
 OK. First let's imagine that it's not a hidden file. Say you create foo
 file (from the command line) in a directory currently shown in dolphin.
 When using inotify, we could get a foo was created signal, but then what?
 KDirLister is going to need more details anyway (size, mimetype, date,
 icon, etc.). To get that, it re-lists the directory. Don't say it could
 just KIO::stat the new file, it becomes very slow if many files get
 created/modified, and it creates much more complex code paths in kdirlister
 which is already complex (it would also need to handle deletion separately).
 Instead we have a single reaction to something changed in this directory
 : re-list it and update it to show the changes (after basically diff'ing
 the new listing and the old listing).

Handling delete should be much simpler than adds as you do not need to lookup 
any new information, so avoiding the whole directory scan on delete sounds 
like a good idea to me in any case, no?

Eike

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

Re: Supported Compilers / C++11 Support in KF5

[Rolf Eike Beer]

Am Montag, 22. Juli 2013, 19:14:10 schrieb Alexander Neundorf:
 On Sunday 21 July 2013, Rolf Eike Beer wrote:

  I totally agree with that. As I said: detection of this is trivial at
  CMake time, maybe I get my C++ feature detection package ready even to be
  included in 2.8.12, and if not the stuff can easily be extracted. And
 
 Yes, that would be great :-)
 You still wanted to have a rweview, right ?

Sure. ;)

Eike

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

Re: Supported Compilers / C++11 Support in KF5

[Rolf Eike Beer]

Volker Krause wrote:

 - GCC = 4.5

 - override

Explicit virtual overrides require g++ 4.7:

http://gcc.gnu.org/projects/cxx0x.html

This is trivially to work around by a CMake time check and then just define 
override to empty.

Eike

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

Re: Supported Compilers / C++11 Support in KF5

[Rolf Eike Beer]

Am Sonntag, 21. Juli 2013, 14:11:07 schrieb Volker Krause:
 On Sunday 21 July 2013 13:52:06 Rolf Eike Beer wrote:
  Volker Krause wrote:
   - GCC = 4.5
   
   - override
  
  Explicit virtual overrides require g++ 4.7:
  
  http://gcc.gnu.org/projects/cxx0x.html
 
 you are right, it's also what all other sources referenced in
 http://article.gmane.org/gmane.comp.kde.devel.frameworks/3652 say, no idea
 how that slipped in then, sorry.
 
  This is trivially to work around by a CMake time check and then just
  define
  override to empty.
 
 sure, for KF5 this is not really a problem since Qt provides corresponding
 macros already. I was hoping for real unconditional usage though, since
 Akonadi will need to stay backward-compatible with Qt4 for a while longer,
 and I wanted to avoid to have to basically copy qcompilerdetection.h for
 that. Not to mention that override looks much nicer than
 Q_DECL_OVERRIDE ;)

I totally agree with that. As I said: detection of this is trivial at CMake 
time, maybe I get my C++ feature detection package ready even to be included 
in 2.8.12, and if not the stuff can easily be extracted. And afterwards you 
just add -Doverride= to CMAKE_CXX_FLAGS and everything is fine.

Eike

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

Re: Review Request 110687: DrKonqi should check for disabled version as the very first step in the reporting assistant.

[Rolf Eike Beer]

Martin Gräßlin wrote:


Could you please get some feedback from packagers. I'm not sure
whether they like words like unmaintained and upgrade. The fact
that we as upstream don't accept bugs doesn't mean it's unmaintained
by the distro and it's not said that one could upgrade (think of
Debian Stable).


I suggest something like try out a more recent version, maybe the bug 
has already been fixed.


Eike

Re: KDE Workspace broken due to upstream CMake changes

[Rolf Eike Beer]

Am 27.05.2013 09:13, schrieb Ben Cooksley:

Hi all,

It seems that a recent upstream change in CMake has now broken the
build of KDE Workspace. Can someone please fix or prod CMake upstream
into revising their policies?

The lack of warning here concerning the change is a little irritating.

-- Looking for XkbLockModifiers in X11
CMake Error at CMakeLists.txt:10 (ADD_EXECUTABLE):
  Target cmTryCompileExec70252 links to item 
/usr/lib64/libXpm.so 
  which has leading or trailing whitespace.  This is now an error 
according

  to policy CMP0004.


CMake Error: Internal CMake error, TryCompile generation of cmake 
failed

-- Looking for XkbLockModifiers in X11 - not found


That's what the policies are for at all ;)

cmake --help-policy CMP0004

So, fix whatever is causing this, and in the meantime use:

cmake_policy(SET CMP0004 OLD)

Eike

Re: Review Request 110633: Don't report crashes if version is disabled in Bugzilla

[Rolf Eike Beer]

Am Samstag 25 Mai 2013, 10:19:37 schrieb Jekyll Wu:

 Well, DrKonqi uses Product.get API to fetch product information from
 bugs.kde.org, including all available versions (either active or disabled).
 But that is not the point of those escaped reports which shouldn't be
 accepted. Even if DrKonqi doesn't know any version information on
 bugs.kde.org, when it sends a request against some disabled version,
 bugs.kde.org (since upgrading to bugzilla 4.2.5) should detect and reject
 the request, then send back a error message to drkonqi, like the screenshot
 in https://bugs.kde.org/attachment.cgi?id=78600action=edit.
 
 Now this expected rejecting behavior works for versions like 4.9.0 or
 4.10.60 (no whitespace), but not for 4.8.5 (4.8.5) (containing
 whitespace). It might be that DrKonqi sends version containing whitespace
 in a wrong way, or that bugzilla deals with version containing white space
 in a wrong way. But since Drknonqi also sends distribution information
 (like Ubuntu Packages) containing whitespace in the same way and bugzilla
 deals with that correctly, I'm inclined to think something goes wrong in
 bugzilla when dealing with versions.

Maybe it's not the space, but (). Maybe it is used as regex or something?

Eike

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

Re: libs/kworkspace/kdisplaymanager.cpp mess

[Rolf Eike Beer]

Am Dienstag, 7. Mai 2013, 22:21:46 schrieb Àlex Fiestas:
 Would be nice to have some kind of unittests on this, that will make a
 different with the current implementation (which afaik it has none)

Even better would be to have them first to prove that nothing changed
afterwards ;)

Eike

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

Re: NepomukCore - Do not merge KDE/4.10 into master

[Rolf Eike Beer]

Am Dienstag, 7. Mai 2013, 00:30:25 schrieb Albert Astals Cid:
 El Dissabte, 4 de maig de 2013, a les 14:01:45, Vishesh Handa va escriure:
  Hey everyone
  
  As you might have heard there was a fiasco in the nepomuk-core repository
  where the 'master' branch was accidentally merged into KDE/4.10. Since
  then
  the system admins had to do a hard reset to v4.10.2 and I had to manually
  cherry-pick a lot of the commits.
  
  I do not want anyone to merge KDE/4.10 into master. It will lead will a
  number of duplicate commits, and considering we already have a LOT of
  duplicates I do not want any more.
 
 Can't you just merge the branches, then rebase -i and in the rebase actually
 remove all the duplicated commits?
 
 Asking for people not to merge is hard, people won't read this, so either
 you enforce it somehow or merge it first like i said and make sure you
 kill the duplicate commits.

You probably meant

git checkout master
git merge -s ours KDE/4.10

Eike

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

Re: Review Request 110271: libusb-1 support in kcmusb (kinfocenter)

[Rolf Eike Beer]

Am 02.05.2013 15:49, schrieb Max Brazhnikov:


Use libusb-1 to query info about usb devices in kinfocenter.
Remove *BSD specific code: it doesn't work on all supported FreeBSD
versions. In principle it can be saved for NetBSD, but NetBSD could
use libusb-1, thus drop it for simplification.
Remove polling and use DeviceNotifier instead.
Add FindLibUSB-1.cmake


First, the pkg-config stuff can be used on Windows. If the stuff isn't 
there it will simply do nothing, but it could.


Then, find_library should never write it's stuff to a *_LIBRARIES 
variable as it will always only find one lib. The libraries variable 
should be composed from the libs found before and should not be a cached 
variable.


Finally, if the .pc provides a version you should use the newer 
interface of FPHSA that also allows version selection.


Eike

Re: Review Request 110091: clean up and update FreeBSD support for kinfocenter

[Rolf Eike Beer]

Am 02.05.2013 15:12, schrieb Max Brazhnikov:

On Wed, 01 May 2013 14:44:39 +0200 Rolf Eike Beer wrote:

On April 20, 2013, 2:11 p.m., Rolf Eike Beer wrote:

kinfocenter/Modules/base/info_fbsd.cpp, line 136
http://git.reviewboard.kde.org/r/110091/diff/1/?file=139992#file139992l
ine136 
Why not just use QProcess here to get the result? I fear this 
stuff
dates back to QT(=3) times where this probably had issues, 
but

that isn't true anymore.

GetInfo_ReadfromPipe already uses QProcess.


Hm, ok. But the 21 will not work, as that is shell syntax for 
redirects and
I'm not sure if QProcess will unterstand that. What I basically had 
in mind
was to pass a QStringList to the functions so that the arguments are 
already

properly separated. Basically what the commenter here had in mind
(info_fbsd.cpp):

// TODO: GetInfo_ReadfromPipe should be improved so that we could 
pass the

program name and its
//   arguments to it and remove most of the code below.

So it would be nice if you could clean up that, too.


Ok, here's a patch for GetInfo_ReadfromPipe only:
http://people.freebsd.org/~makc/patches/read_from_pipe.diff


Looks good to me (although I have not tested it). Small note: there is 
a contructor of QStringList taking a QString so if you have a list with 
only one item you don't need to construct an empty list and then add 
something using operator.


Side-note: there is a comment talking about using /proc/pci if lspci is 
not found. Not that newer kernels do have a /proc/pci at all ;)



While you're at it you
can then fix the type (devies - devices), too ;)


I've removed those comments completely.


Another solution for that problem ;)

Eike

Re: Review Request 110091: clean up and update FreeBSD support for kinfocenter

[Rolf Eike Beer]

  On April 20, 2013, 2:11 p.m., Rolf Eike Beer wrote:
   kinfocenter/Modules/base/info_fbsd.cpp, line 136
   http://git.reviewboard.kde.org/r/110091/diff/1/?file=139992#file139992l
   ine136  
   Why not just use QProcess here to get the result? I fear this stuff
   dates back to QT(=3) times where this probably had issues, but
   that isn't true anymore.
 GetInfo_ReadfromPipe already uses QProcess.

Hm, ok. But the 21 will not work, as that is shell syntax for redirects and 
I'm not sure if QProcess will unterstand that. What I basically had in mind 
was to pass a QStringList to the functions so that the arguments are already 
properly separated. Basically what the commenter here had in mind 
(info_fbsd.cpp):

// TODO: GetInfo_ReadfromPipe should be improved so that we could pass the 
program name and its
//   arguments to it and remove most of the code below.

So it would be nice if you could clean up that, too. While you're at it you 
can then fix the type (devies - devices), too ;)

Eike

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

Re: Review Request 110110: Add Musics , Downloads , Videos, Pictures places bookmarks to kfileplacesmodal

[Rolf Eike Beer]

Am Sonntag 21 April 2013, 02:54:30 schrieb Ömer Fadıl Usta:
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/110110/
 ---
 
 Review request for kdelibs.
 
 
 Description
 ---
 
 Add Musics , Downloads , Videos, Pictures places bookmarks to
 kfileplacesmodal Patch from Mandriva Linux

Yeah, Windows XP feeling finally! Where can one disable this?

Eike

Re: Review Request 110091: clean up and update FreeBSD support for kinfocenter

[Rolf Eike Beer]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110091/#review31332
---



cmake/modules/FindLibdevinfo.cmake
http://git.reviewboard.kde.org/r/110091/#comment23350

Why is this necessary at all? If you don't have a strong reason for it just 
drop it.



cmake/modules/FindLibdevinfo.cmake
http://git.reviewboard.kde.org/r/110091/#comment23356

Please read Modules/readme.txt from CMake about how to name such variables. 
Short: this should be DEVINFO_INCLUDE_DIR, and you should have a 
DEVINFO_INCLUDE_DIRS later, which is not cached.



cmake/modules/FindLibdevinfo.cmake
http://git.reviewboard.kde.org/r/110091/#comment23352

Has the header a version number one could parse out and use?




kinfocenter/Modules/base/CMakeLists.txt
http://git.reviewboard.kde.org/r/110091/#comment23351

The TODO comment above can go away now I think.



kinfocenter/Modules/base/info_fbsd.cpp
http://git.reviewboard.kde.org/r/110091/#comment23353

Why not just use QProcess here to get the result? I fear this stuff dates 
back to QT(=3) times where this probably had issues, but that isn't true 
anymore.



kinfocenter/Modules/base/info_fbsd.cpp
http://git.reviewboard.kde.org/r/110091/#comment23354

QProcess here, too. Otherwise it may be a good idea to just put your patch 
in now as it is and do a followup patch that just kills that function and 
replaces it with QProcess, or keeps that functions and gives it a proper 
QProcess-based interface, i.e. separate arguments passed as a QStringList and 
such.



kinfocenter/Modules/base/info_fbsd.cpp
http://git.reviewboard.kde.org/r/110091/#comment23355

This line is flagged to have inconsistent whitespace.



kinfocenter/Modules/info/CMakeLists.txt
http://git.reviewboard.kde.org/r/110091/#comment23357

I miss a include_directories(${DEVINFO_INCLUDE_DIRS}) or something like 
that here.


- Rolf Eike Beer


On April 19, 2013, 10:14 p.m., Max Brazhnikov wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/110091/
 ---
 
 (Updated April 19, 2013, 10:14 p.m.)
 
 
 Review request for kde-workspace.
 
 
 Description
 ---
 
 Add FindLibdevinfo.cmake
 Clean up info_fbsd.cpp and utilize PCIUtils
 
 
 Diffs
 -
 
   cmake/modules/FindLibdevinfo.cmake e69de29 
   kinfocenter/Modules/base/CMakeLists.txt 2b3c34e 
   kinfocenter/Modules/base/info_fbsd.cpp 6bbaa1a 
   kinfocenter/Modules/info/CMakeLists.txt dba6bc7 
 
 Diff: http://git.reviewboard.kde.org/r/110091/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Max Brazhnikov
 

Re: Review Request 110042: Find Qt5 version of DBusMenuQt

[Rolf Eike Beer]

Am Dienstag 16 April 2013, 13:26:23 schrieb Frederik Gladhorn:
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/110042/
 ---
 
 Review request for kdelibs.
 
 
 Description
 ---
 
 Build fix for dbusmenu qt5 changes.
 This appends the 5 to include path and lib dir in the find module.
 Also rename the whole thing to not conflict with the Qt 4 version.

Since DBusMenuQt5 is obviously a rather new thing I would vote for putting a 
DBusMenuQt5Config.cmake into that project itself and install that. That would 
allow everyone to use it with CMake once the module itself is installed, 
without any need for a Find*.cmake module.

Eike

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

Re: [kdelibs] kded: Disable KHostnameD in KDED

[Rolf Eike Beer]

Am 15.04.2013 12:43, schrieb Àlex Fiestas:

Git commit 8d99f863724c6fe76d008da4455fa177af2ee3ee by Àlex Fiestas.
Committed on 15/04/2013 at 12:24.
Pushed by afiestas into branch 'master'.

Disable KHostnameD in KDED

KHostnameD monitors the hostname by polling gethostname every 5 
seconds

to adapt some bits of the environment in case the hostname changes. In
systems with only a local X server this does not matter at all. In
systems using remote X you should not change your hostname, period.
In most distributions changing your hostname no longuer breaks X 
access

for local applications since xhost is configured correctly. In summary
there is NO reason to keep this around.

Besides that, kdontchangethehostname (binay that is called when the
hostname is changed) does not work if your DISPLAY env is like :0
which is the format that can be found in all distributions nowdays
(old format was domain/[unix,tcp,...]:0).

Even fixing it, most distributions use a global Xauthority pointed by
the DM or XAUTHORITY env which points to either /run/*dm/... or in
/tmp/user Meaning that the most important adaptation to the new
hostname that kdontchangethehostname does won't work either.

If this is ever enabled again the polling should be removed by either
using systemd-hostnamed or inotify+/etc/hostname but polling won't
be accepted.

This commits only disables the code, if nobody complains I will remove
the full code before 4.11 is out.

CCMAIL:kde-core-devel@kde.org

M  +1-1kded/kded.cpp

http://commits.kde.org/kdelibs/8d99f863724c6fe76d008da4455fa177af2ee3ee

diff --git a/kded/kded.cpp b/kded/kded.cpp
index b594692..b611972 100644
--- a/kded/kded.cpp
+++ b/kded/kded.cpp
@@ -842,7 +842,7 @@ public:
 #endif

   if (bCheckHostname)
-(void) new KHostnameD(HostnamePollInterval); // Watch
for hostname changes
+// (void) new KHostnameD(HostnamePollInterval); // Watch
for hostname changes

   kded-initModules();
} else


Nice, now we have

if (bCheckHostname)
   kded-initModules();

This can't be right.

Eike

Re: Progress notifications questions

[Rolf Eike Beer]

Am 04.04.2013 11:31, schrieb Mirko Boehm:

Hi guys,

Danimo and I have been working on desktop progress feedback for 
Creator
in Plasma desktop and Unity. See the attached screenshot for the 
current

state. After discussions in #kde-devel, I implemented an adapted KJob
and used KUiServerJobTracker to talk to the notification area. So far,
so good, bu there are some specific questions:
Have a look at platform_plasmaplugin.cpp here:

https://github.com/danimo/qt-creator

- in the first screen shot (the one numbered 2 ;-) ), how can I set a
displayed text next to the Qt Creator icon? I would like to show the
project name and the current operation (build/rebuild/clean/...).

- The tooltip for the notification area shows 1 running job (0b/s).
That is of course nonsense, since this is not an I/O operation. How 
can

I get it to display 1 running job (32% done)?

- Is there a way to show detail text for an operation? Something like
the number of warnings and errors?


emit description(...)

Eike, still waiting for bug 312796 and 311413 to get any developer 
attention

Password strengh meter in KNewPasswordDialog

[Rolf Eike Beer]

Hi all,

the current issue of (German) Linux Magazin has an article comparing some 
GnuPG frontends. One issue discussed there is the password strength meter 
that gives e.g. 25% strength indication for things like 123456789. I don't 
know about Kleopatra, but KGpg uses KNewPasswordDialog and it's strength meter 
for this. I propose to change the algorithm used to calculate the password 
strength to remove key sequences from the length calculation of the 
password, i.e. 123 has the same length as 1. Also punish all passwords harder 
that do not contain all types of characters, so a password containing only 
lowercase characters and numbers needs to be much longer than one also 
containing specials and uppercase characters.

I've attached my strength test program containing both the old and the 
proposed new version of the code. I've tested the new version in 2 variants, 
once with and without the call to toLower() before checking for sequences. 
These are some test passwords I used, mostly some examples of simple 
passwords users will use. The last one is a scrambled version of a password I 
saw used somewhere (i.e. every letter replaced with something from the same 
character class to retain the score) that was not totally obvious.

 old nEw new
abcdef45  12  12
abcdefghi 72  22  22
1 12   1   1
1215   2   2
123   17   2   2
1234  20   2   2
12345 22   2   2
12345625   2   2
123456789 32   2   2
qwertz45  25  25
1234test  40  20  20
test1234  30  10  10
a1b2c3d4 100  60  60
a1b2c3d4e5   100  85  85
a1b2c3d4e5f6 100 100 100
a1a1a1a1  40  20  20
  30   2   2
KKvfnDd.  90  57  55

Also I propose to change the color of the strength indicator to red below 50% 
and to yellow below 75%. Since this does not affect any strings and improves 
security I would also push this into 4.10 in noone objects.

Comments?

Eike#include QString
#include QDebug

const int reasonablePasswordLength = 8;

int effectivePasswordLength(const QString password)
{
	enum Category {
		Digit,
		Upper,
		Vowel,
		Consonant,
		Special
	};
	
	Category previousCategory = Vowel;
	QString vowels(aeiou);
	int count = 0;
	int catCount = 0;
	unsigned int catMask = 0;
	
	for (int i = 0; i  password.length(); ++i) {
		QChar currentChar = password.at(i);
		if (!password.left(i).contains(currentChar)) {
			Category currentCategory;
			switch (currentChar.category()) {
case QChar::Letter_Uppercase:
	currentCategory = Upper;
	break;
case QChar::Letter_Lowercase:
	if (vowels.contains(currentChar)) {
		currentCategory = Vowel;
	} else {
		currentCategory = Consonant;
	}
	break;
case QChar::Number_DecimalDigit:
	currentCategory = Digit;
	break;
default:
	currentCategory = Special;
	break;
			}
			switch (currentCategory) {
case Vowel:
	if (previousCategory != Consonant) {
		++count;
	}
	break;
case Consonant:
	if (previousCategory != Vowel) {
		++count;
	}
	break;
default:
	if (previousCategory != currentCategory) {
		++count;
	}
	break;
			}
			previousCategory = currentCategory;
			if (!(catMask  (1  currentCategory))) {
++catCount;
catMask |= (1  currentCategory);
			}
		}
	}
	// passwords that have many category changes but few categories are weaker
	return (count * catCount) / 5;
}

int passwordStrength(const QString password)
{
	QString sPass = password.simplified().toLower();
	
	if (sPass.length()  2)
		return sPass.length();
	
	int i = 0;
	while (i  sPass.length()) {
		// duplicate characters do not improve the length
		if (sPass[i] == sPass[i + 1]) {
			sPass.remove(i + 1, 1);
			continue;
		}

		// the sequence detection is only reliable in the ASCII range
		if (!sPass[i].isLetterOrNumber()) {
			++i;
			continue;
		}
		
		if (sPass[i].unicode() == sPass[i + 1].unicode() + 1 || sPass[i].unicode() == sPass[i + 1].unicode() - 1) {
			// Remove the old one here. Otherwise we would not catch 123 as a sequence
			sPass.remove(i, 1);
			continue;
		}
		++i;
	}
	
	int pwstrength = (20 * sPass.length() + 80 * effectivePasswordLength(password)) / qMax(reasonablePasswordLength, 2);
	if (pwstrength  0) {
		pwstrength = 0;
	} else if (pwstrength  100) {
		pwstrength = 100;
	}
	
	return pwstrength;
}

int old_effectivePasswordLength(const QString password)
{
	enum Category {
		Digit,
		Upper,
		Vowel,
		Consonant,
		Special
	};
	
	Category previousCategory = Vowel;
	QString vowels(aeiou);
	int count = 0;
	
	for (int i = 0; i  password.length(); ++i) {
		QChar currentChar = password.at(i);
		if (!password.left(i).contains(currentChar)) {
			Category currentCategory;
			switch (currentChar.category()) {
case QChar::Letter_Uppercase:
	currentCategory = Upper;
	break;
case QChar::Letter_Lowercase:
	if (vowels.contains(currentChar)) {
		currentCategory = Vowel;
	} else {
		

Re: Password strengh meter in KNewPasswordDialog

[Rolf Eike Beer]

Am Mittwoch 03 April 2013, 18:47:17 schrieb Cristian Tibirna:
 On Wednesday 03 April 2013 22:39:47 Rolf Eike Beer wrote:
  Hi all,
  
  the current issue of (German) Linux Magazin has an article comparing some
  GnuPG frontends. One issue discussed there is the password strength
  meter
  that gives e.g. 25% strength indication for things like 123456789. I don't
  know about Kleopatra, but KGpg uses KNewPasswordDialog and it's strength
  meter for this. I propose to change the algorithm used to calculate the
  password strength to remove key sequences from the length calculation of
  the password, i.e. 123 has the same length as 1. Also punish all passwords
  harder that do not contain all types of characters,
 
 http://xkcd.com/936/
 
  so a password
  containing only lowercase characters and numbers needs to be much longer
  than one also containing specials and uppercase characters.
 
 Really, this whole can be short because has mixed types of characters
 nonsense has to die.

Not short, just shorter. So this boils down to the question: how can we count 
the bits of entropy?

 There is a math theory behind password strength. There might even be
 libraries capable of measuring this properly.
 
 IMH (non-contributor) O, we should try to reuse here.

Adding dependencies would only affect 4.11, but I guess even for that the time 
may already be too short. Not that it wouldn't be a good idea for 4.12 if it's 
worth the effort.

Eike

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

Re: Review Request 109549: port KRun away from KProcess

[Rolf Eike Beer]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109549/#review29391
---



kio/kio/krun.cpp
http://git.reviewboard.kde.org/r/109549/#comment21955

Whitespacing around braces is inconsistent.



kio/kio/krun.cpp
http://git.reviewboard.kde.org/r/109549/#comment21954

Trailing whitespace


- Rolf Eike Beer


On March 17, 2013, 4:19 p.m., Martin Tobias Holmedahl Sandsmark wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/109549/
 ---
 
 (Updated March 17, 2013, 4:19 p.m.)
 
 
 Review request for KDE Frameworks, kdelibs and David Faure.
 
 
 Description
 ---
 
 Port KRun to use QProcess instead of KProcess.
 
 Instead of passing around KProcess instances, we simply pass the command with 
 arguments and the working directory.
 
 
 Diffs
 -
 
   kio/kio/krun.cpp 76b7385 
   kio/kio/krun_p.h 01abb69 
 
 Diff: http://git.reviewboard.kde.org/r/109549/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Tobias Holmedahl Sandsmark
 

Re: cxx11-cmake-modules in kdereview

[Rolf Eike Beer]

Am Freitag, 1. März 2013, 16:06:38 schrieb Ivan Čukić:
 Hi all!

 The CMake modules for detecting C++11 features are going to get in kdereview
 soon. The target for the repository is kdesupport.

There is a feature request for CMake itself to provide such a module, and I
have half of the implementation already done. It will not go into 2.8.11, but
the aim is definitely 2.8.12.

So if it is possible to have something similar achieved without yet another
module I would definitely say to avoid introducing this new module. If there
are 2 users or maybe 4 I would say put those few files directly in those
repositories, replace it with the ones from upstream CMake when it arrives
there and drop it altogether once you bump the required CMake version to
2.8.12 or something.

Eike

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

Re: cmake.git (Re: [Kde-pim] Boost vs cmake 2.8.8 vs kdepimlibs master)

[Rolf Eike Beer]

Am Montag, 17. Dezember 2012, 23:11:14 schrieb David Faure:
 [How many lists do I need to read this thread on? Cutting the cross-posting
 off]
 
 On Monday 17 December 2012 17:46:51 Laszlo Papp wrote:
  PS.: If only the entry barrier was not so high for even a basic tool like
  cmake. It is certainly not a problem for me as an arch user, but I can
  understand this a -1 for many. This is of course off-topic here.
 
 It takes exactly 4 lines in your kdesrc-buildrc to get the latest cmake
 updated and built at the same time as kdelibs-frameworks:
 
 module cmake-git
   repository git://cmake.org/cmake.git
   ordering-tier 0
 end module
 
 This is a high entry barrier? Pff.

wget http://www.cmake.org/files/v2.8/cmake-2.8.10.2-Linux-i386.tar.gz
tar xf cmake-2.8.10.2-Linux-i386.tar.gz

Eike

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

Re: Review Request: Do not wait for remaining Dr.Konqi instances without timeout when ending the KDE session

[Rolf Eike Beer]

The only downside is some backtrace might be lost. But I don't think
that is a big deal. Crashes during shutdown are rare cases nowadays(I
hope I'm right), and users noticing those crashes only hours later 
are

the rare case in rare cases (but very annoying).


I regularly get them, most times something Kontact related. What about 
just saving the backtrace to disk? Next step could be on next startup to 
check if there are automatically saved backtraces and ask if to 
drop/save/report them.

Re: Review Request: Do not wait for remaining Dr.Konqi instances without timeout when ending the KDE session

[Rolf Eike Beer]

Am , schrieb Jekyll Wu:

于 2012年12月10日 19:27, Rolf Eike Beer 写道:
The only downside is some backtrace might be lost. But I don't 
think
that is a big deal. Crashes during shutdown are rare cases 
nowadays(I
hope I'm right), and users noticing those crashes only hours later 
are

the rare case in rare cases (but very annoying).


I regularly get them, most times something Kontact related. What 
about
just saving the backtrace to disk? Next step could be on next 
startup to

check if there are automatically saved backtraces and ask if to
drop/save/report them.


Just to make it clear, when I say some backtrace might be lost I
mean under the cases that bug 126073 complains: Users think their
systems will shutdown as expected but hours later find it hasn't
because startkde waits for Dr.Konqi forever. The proposed change 
will

not stand in the way of users who already notice the crash at the
first place and want to save/report the backtrace.

Saving backtrace to disk is of course good, but unfortunately
Dr.Konqi doesn't seem to have a dbus call for that operation at the
monent. As for Next step, see the comment from George Kiagiadakis.
That is not an easy task and might take a long time to implement or
never.


Add one. ;)


On the other hand, bug 126073 is a big annoyance to anyone who has
ran across it. And it has been there and annoying users for a long
time. Comparing the big benefit and small lost of the proposed 
change,
I think that Some backtraces might be lost is really not a big 
deal.


Agreed. But please set the timeout to something like 5 minutes. When I 
want to shutdown I want to shutdown, maybe because my batteries are 
nearly empty. Waiting for an hour ist far too long IMHO.


Eike

Re: Review Request: parenthesis and not null checking

[Rolf Eike Beer]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107213/#review21785
---


A simple way to verify if this is correct is check if you and the compiler 
agree on the code. Run make -n in the build tree to get the full compiler 
command line, then insert a -S (assuming you are using gcc) and change the -o 
to point to a temporary file. This will output the assembler code. Do this with 
and without your modifications and look if the result is still the same.

- Rolf Eike Beer


On Nov. 5, 2012, 3:13 p.m., Jaime Torres Amate wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/107213/
 ---
 
 (Updated Nov. 5, 2012, 3:13 p.m.)
 
 
 Review request for kdelibs.
 
 
 Description
 ---
 
 place some parenthesis around ! operators, with less priority than ^ 
 operators.
 place some parenthesis around = inside conditions 
 check for n not being null before using it
 simplify if (a==true) return true else return false;
 
 
 Diffs
 -
 
   khtml/khtml_caret.cpp 45fd90c 
   khtml/rendering/render_inline.cpp acfc1e4 
   khtml/rendering/render_object.cpp f37627c 
   solid/solid/backends/wmi/wmiopticaldisc.cpp c6e390f 
 
 Diff: http://git.reviewboard.kde.org/r/107213/diff/
 
 
 Testing
 ---
 
 I've been using this code several weeks.
 
 
 Thanks,
 
 Jaime Torres Amate
 

Re: CMake 2.8.8 will be required for kdelibs 4.10 starting October 30th

[Rolf Eike Beer]

Am Dienstag 30 Oktober 2012, 20:24:19 schrieb Alexander Neundorf:
 On Friday 26 October 2012, Alexander Neundorf wrote:
  On Thursday 18 October 2012, Alexander Neundorf wrote:
   Hi,
   
   in kdelibs we require since more than 2 years cmake .2.6.4, since then
   many improvements and fixes have gone into cmake, and we cannot make use
   of them.
   
   These are e.g.
   * builtin automoc
   * support for creating proper Config.cmake files
   * ${CMAKE_CURRENT_LIST_DIR}
   and many many more.
   
   So, after the discussion we had here on this list,
   starting October 30th CMake 2.8.8 or newer will be required for building
   kdelibs 4.10.
   
   If it is not yet available in your distribution, simply wget and untar
   it:
   
   $ wget http://www.cmake.org/files/v2.8/cmake-2.8.9-Linux-i386.tar.gz
   $ su
   $ cd /opt
   $ tar -zxvf path/to/cmake-2.8.9-Linux-i386.tar.gz
   ...
   $ /opt/cmake-2.8.9-Linux-i386/bin/cmake ...all your arguments
  
  just as a reminder, this is Tuesday next week.
 
 Ok, I pushed this change to the kdelibs 4.10 branch.
 
 Let me know if something doesn't build anymore.
 
 Especially for the Windows team: I removed our version of FindOpenSSL.cmake,
 so we use the one coming with cmake now. I don't remember exactly who it
 was, but somebody reported that the verison coming with cmake would
 actually work better under Windows than our version. Now, there is a small
 potential incompatibility in this file: the variable
 ${OPENSSL_EAY_LIBRARIES} does not exist anymore, but the generic
 ${OPENSSL_LIBRARIES} variable should contain everything that's needed, so
 if there are no special tests for this variable it should still work.

Should FindFlex.cmake be removed in favor of upstream FindFLEX.cmake?

Eike

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

Re: Moving libkfacebook to extragear

[Rolf Eike Beer]

Am Samstag, 27. Oktober 2012, 11:00:42 schrieb Martin Klapetek:
 Hi,
 
 I'd like to move libkfacebook, the foundation for akonadi-facebook
 resource, into extragear. It's been in use for a while, lots of distro ship
 it bundled with akonadi-facebook resource, which is now becaming part of
 kdepim-runtime for 4.10 with libkfacebook as optional compile-time
 dependency.
 
 I'd like to ask for a review of this library, currently in kdereview -
 https://projects.kde.org/projects/kdereview/libkfacebook

I don't have any knowledge about this, but recently there was some library 
named libkgoogle which has been renamed to something other to avoid possible 
trademark hazard. This has been in the thread Review request: moving 
libkgoogle to extragear starting 2012-05-26.

Eike

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

Re: Review Request: Add pkgconfig hints to FindSamba.cmake

[Rolf Eike Beer]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106861/#review20357
---



cmake/modules/FindSamba.cmake
http://git.reviewboard.kde.org/r/106861/#comment16103

Why the if? If there is no pkgconfig found it the macro will just do 
nothing. And pkgconfig could even work on Windows if it is properly installed.



cmake/modules/FindSamba.cmake
http://git.reviewboard.kde.org/r/106861/#comment16101

trailing whitespace



cmake/modules/FindSamba.cmake
http://git.reviewboard.kde.org/r/106861/#comment16104

That should be HINTS:

From cmake help:

Search the paths specified by the HINTS option. These should be paths 
computed by system introspection, such as a hint provided by the location of 
another item already found. Hard-coded guesses should be specified with the 
PATHS option.



cmake/modules/FindSamba.cmake
http://git.reviewboard.kde.org/r/106861/#comment16102

trailing whitespace



cmake/modules/FindSamba.cmake
http://git.reviewboard.kde.org/r/106861/#comment16105

HINTS


- Rolf Eike Beer


On Oct. 14, 2012, 11:43 p.m., Rex Dieter wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/106861/
 ---
 
 (Updated Oct. 14, 2012, 11:43 p.m.)
 
 
 Review request for kdelibs.
 
 
 Description
 ---
 
 Add pkgconfig hints to FindSamba.cmake, helps find samba4 libs on 
 non-standard-ish locations.
 
 
 Diffs
 -
 
   cmake/modules/FindSamba.cmake 16522c6 
 
 Diff: http://git.reviewboard.kde.org/r/106861/diff/
 
 
 Testing
 ---
 
 Tested on fedora 18 against samba-4.0-rc2
 
 
 Thanks,
 
 Rex Dieter
 

Re: Requiring cmake 2.8.9 for kdelibs 4.10 ?

[Rolf Eike Beer]

Am Samstag 29 September 2012, 10:36:55 schrieb Alexander Neundorf:
 Hi,
 
 since May 2010 kdelibs requires CMake 2.6.4 for building.
 
 This version is quite old in the meantime, and we are missing on new CMake
 features and also run into problems with some cmake modules where we have an
 own copy, which is not forward compatible to the new versions coming with
 CMake.
 
 So, I'd like to raise the required minimum version of CMake for kdelibs 4.10
 to 2.8.9 (released beginning of August).
 
 I know this will cause some effort, because I guess only few distributions
 already come with CMake 2.8.9, but doing this once again after 2 1/2 years
 seems acceptable for me.
 
 Objections ?

None from my side, but I don't think that surprises anyone. 4.10 will likely 
require some other updated stuff which may cause deeper trouble in the system 
(e.g. some updated libs). Changing the CMake version will not change anything. 
It's not even required at runtime, so I don't think that will cause trouble to 
anyone.

Eike

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

Re: Requiring cmake 2.8.9 for kdelibs 4.10 ?

[Rolf Eike Beer]

Am Samstag 29 September 2012, 11:48:16 schrieb André Wöbbeking:
 Hi Alex,

 On Saturday 29 September 2012 10:36:55 Alexander Neundorf wrote:
  I know this will cause some effort, because I guess only few distributions
  already come with CMake 2.8.9, but doing this once again after 2 1/2 years
  seems acceptable for me.

 Do you really need 2.8.9 or would 2.8.8 from April also be sufficient? The
 older version would probably cause less trouble?!?

What trouble would it cause?

Eike

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

Re: Requiring cmake 2.8.9 for kdelibs 4.10 ?

[Rolf Eike Beer]

Am Samstag 29 September 2012, 12:12:43 schrieb André Wöbbeking:
 On Saturday 29 September 2012 11:59:04 Rolf Eike Beer wrote:
  Am Samstag 29 September 2012, 11:48:16 schrieb André Wöbbeking:
   Hi Alex,
  
   On Saturday 29 September 2012 10:36:55 Alexander Neundorf wrote:
I know this will cause some effort, because I guess only few
distributions
already come with CMake 2.8.9, but doing this once again after 2 1/2
years
seems acceptable for me.
  
   Do you really need 2.8.9 or would 2.8.8 from April also be sufficient?
   The
   older version would probably cause less trouble?!?
 
  What trouble would it cause?

 Well it's one more dependency to fulfill before you can build KDE. Probably
 no problem for most people here but maybe for distributions or users.

If distros go and upgrade their KDE tarballs to get the new version, what more
hassle is it to just drop in a new CMake tarball before? CMake doesn't have
any dependency changes in the last time I know of and also ships everything
needed inside it's own tarball. So you are right that this is one more
package, but I bet that KDE SC 4.10 would require other things to be upgraded
too. But CMake will be the easiest thing to upgrade IMHO.

Eike

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

Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore

[Rolf Eike Beer]

 * If all available sessions are unselected,
 presssing Restore Session button will behave like Do not Restore.

What about just disabling Restore Session then?

Eike

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

Re: Review Request: konqueror: strip new line character from URL on paste automatically

[Rolf Eike Beer]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106184/#review17997
---



konqueror/src/konqcombo.cpp
http://git.reviewboard.kde.org/r/106184/#comment14232

Please normalize the connect:

connect(edit, SIGNAL(textEdited(QString)), this, 
SLOT(slotTextEdited(QString)));


- Rolf Eike Beer


On Aug. 25, 2012, 7:40 p.m., Martin Koller wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/106184/
 ---
 
 (Updated Aug. 25, 2012, 7:40 p.m.)
 
 
 Review request for KDE Base Apps and David Faure.
 
 
 Description
 ---
 
 This patch removes line-breaking chars when an URL is entered.
 Very handy if you paste URLs from e.g. an email which shows the complete URL 
 broken into multiple lines.
 
 
 This addresses bug 159002.
 http://bugs.kde.org/show_bug.cgi?id=159002
 
 
 Diffs
 -
 
   konqueror/src/konqcombo.h 5c86fcd 
   konqueror/src/konqcombo.cpp 8169aec 
 
 Diff: http://git.reviewboard.kde.org/r/106184/diff/
 
 
 Testing
 ---
 
 yes
 
 
 Thanks,
 
 Martin Koller
 

Re: Review Request: Remember current desktop when changing activity

[Rolf Eike Beer]

Am Samstag, 18. August 2012, 20:19:55 schrieb makism:
 Yeah i read a bug report about this (new) behavior. It would be fair to
 support all perceptions of activities (because of their abstract meaning).
 Ivan mentioned that in @4.10 there will be a KCM for activities, i believe
 that we could add some kind of an option.

As I said: adding it to just a config file first would be fine with me.

Eike

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

Re: Review Request: Fix hang in kcm_useraccount

[Rolf Eike Beer]

Am Samstag 11 August 2012, 00:11:37 schrieb Michael Palimaka:
 On 2012-08-10 02:46, Rolf Eike Beer wrote:
  Ehm, no. readLine() unconditionally calls readAll(). But that will also
  clear the input buffer of the process object. So there is nothing left to
  read on the next readLine() or readAll() call.
  
  Eike
 
 So you are saying:
 
 - readAll clears the buffer
 - readLine unconditionally calls readAll
 - readLine may be called multiple times to read multiple lines
 
 Is that correct?

That's how the code looks to me.

 (Again, I note that in testing, calling readAll then readLine is
 successful.)

I.e. readLine() then returns something? Are you sure this has been written by 
the process before readAll() was called? Strange.

Eike

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

Re: Review Request: Fix hang in kcm_useraccount

[Rolf Eike Beer]

Michael Palimaka wrote:
 On 2012-08-09 02:59, Rolf Eike Beer wrote:
  Now we have the following situation in case the program has exited (if not
  the behavior is unchanged):
  
  -we read in one line, if it is empty we break. What happens if the first
  line of output is empty and correct output comes later?
 
 That's why I originally went for readAll.

You could have just said that instead of taking my suggestions ;)

  -the line is not empty, we continue (new check)
  
  -we check if the line is empty (old check)
  
  So, what now?
  
  I would suggest the following:
  
  -readAll()
  -if empty, break
  -find a linebreak, if we have one: unreadLine for everything beyond it,
  cut
  the input line we have
  -move the readLine() and the isEmpty() of the old code together in the
  else
  (i.e. pidExited == NotExited).

 Isn't that what effectively what the original revision does (dropping
 the unread since readLine after readAll is safe)?
 
 If the program has exited:
 - Read all output
 - If no output is found, break
 - If output is found, continue along the original code
 
 The purpose of this change is only to break if the program has exited
 without output.

You have a nice way trying to say I'm a moron. Thank you. ;)

Your patch isn't technically ideal as it does the same operation on data 
multiple times. But seriously: we have wasted way more time and computing 
power by doing this mail thread then the most ideal code solution will ever be 
able to return.

Just one thing: you do unreadLine(line), which adds true as second argument, 
so a needless newline is appended that wasn't there before (and which causes a 
deep copy of all that stuff for nothing). So unreadLine(line, false) should it 
be. And I will not speak up again on this unless explicitely requested to ;)

Eike

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

Re: Review Request: Fix hang in kcm_useraccount

[Rolf Eike Beer]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105895/#review16985
---



kdepasswd/kcm/chfnprocess.cpp
http://git.reviewboard.kde.org/r/105895/#comment13338

Why unread the line if you read it again just 3 lines below? Why not just 
put the readline() below in an else clause?


- Rolf Eike Beer


On Aug. 6, 2012, 1:20 p.m., Michael Palimaka wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/105895/
 ---
 
 (Updated Aug. 6, 2012, 1:20 p.m.)
 
 
 Review request for KDE Base Apps.
 
 
 Description
 ---
 
 When changing the user's full name, chfn may not necessarily produce any 
 output. Since readLine blocks, the kcm may hang.
 
 This change checks if chfn exited without output, and if so, use that exit 
 status.
 
 
 This addresses bug 156396.
 http://bugs.kde.org/show_bug.cgi?id=156396
 
 
 Diffs
 -
 
   kdepasswd/kcm/chfnprocess.cpp 9f75d4aa75b41acec84e7798c789d4226ca3fab9 
 
 Diff: http://git.reviewboard.kde.org/r/105895/diff/
 
 
 Testing
 ---
 
 On a PAM-enabled system:
 * Full name changed successfully when permitted by login.defs
 * Error presented and no change processed with prohibited by login.defs
 
 
 Thanks,
 
 Michael Palimaka
 

Re: playground/games/picmi moved to KDE Review

[Rolf Eike Beer]

Am 25.07.2012 10:50, schrieb Patrick Spendrin:

Am 25.07.2012 10:13, schrieb Andras Mantia:


I keep telling to Patrick to just use Linux, but he is hard headed, 
so he has

to leave with the pain and fix our mess. ;)


:-D
If I ever stop working on KDE on Windows I will switch.


Looks like we will always win ;)

Eike

Re: playground/games/picmi moved to KDE Review

[Rolf Eike Beer]

Am Samstag 21 Juli 2012, 10:48:47 schrieb Jakob Gruber:
 On 07/21/2012 12:02 AM, Raphael Kubo da Costa wrote:
  Jakob Gruber jakob.gru...@gmail.com writes:
  Building with KDE trunk will require the patch from
  http://lists.kde.org/?l=kde-games-develm=134201653803914w=2.
  
  BTW, the config.h part of the patch should go in regardless of the
  rest, as config.h should be the first header included by the source
  files anyway.
 
 Done. I don't understand why krazy tells me to put config.h in angle
 brackets though - I've left it in quotes for now.

Because quotes means take it from the current directory first. The config.h 
you write is in the binary directory, so it should be included with  which 
does not search the current directory first. At least not if you are not using 
MSVC :/

Eike

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

Re: Review Request: kjs: Implement Date.toJSON

[Rolf Eike Beer]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105631/#review16171
---



kjs/CommonIdentifiers.h
http://git.reviewboard.kde.org/r/105631/#comment12766

toJSON is already present and you add toISOString? Are you sure this is the 
right diff?


- Rolf Eike Beer


On July 20, 2012, 6:57 p.m., Bernd Buschinski wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/105631/
 ---
 
 (Updated July 20, 2012, 6:57 p.m.)
 
 
 Review request for kdelibs.
 
 
 Description
 ---
 
 kjs: Implement Date.toJSON
 
 according to ecmascript edition 5.1r6 - 15.9.5.44 
 
 
 Diffs
 -
 
   kjs/CommonIdentifiers.h 8ee40e8 
   kjs/date_object.h ed45720 
   kjs/date_object.cpp 8a1fc2c 
 
 Diff: http://git.reviewboard.kde.org/r/105631/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Bernd Buschinski
 

Re: Review Request: kjs: Implement JSON.parse

[Rolf Eike Beer]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105056/#review15397
---


No functional review, just reading through the code.


kjs/jsonlexer.cpp
http://git.reviewboard.kde.org/r/105056/#comment11995

If we are in parse...?



kjs/jsonlexer.cpp
http://git.reviewboard.kde.org/r/105056/#comment11996

Can those 3 just be merged and use return UChar(cur.uc);?


- Rolf Eike Beer


On July 4, 2012, 10:27 p.m., Bernd Buschinski wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/105056/
 ---
 
 (Updated July 4, 2012, 10:27 p.m.)
 
 
 Review request for kdelibs.
 
 
 Description
 ---
 
 kjs: Implement JSON.parse
 
 
 Diffs
 -
 
   kjs/CMakeLists.txt 1188064 
   kjs/interpreter.cpp cf1acf1 
   kjs/json_object.h PRE-CREATION 
   kjs/json_object.cpp PRE-CREATION 
   kjs/jsonlexer.h PRE-CREATION 
   kjs/jsonlexer.cpp PRE-CREATION 
   kjs/libkjs.map e9f679f 
 
 Diff: http://git.reviewboard.kde.org/r/105056/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Bernd Buschinski
 

Re: Compiler version

[Rolf Eike Beer]

Am Donnerstag, 28. Juni 2012, 10:20:42 schrieb Thiago Macieira:
 On quinta-feira, 28 de junho de 2012 10.14.03, Ivan Cukic wrote:
  Well, nullptr is a compile time check, right (like explicit override)? So,
  you  compile your code with a compiler that supports it, making your code
  safe in that aspect, while someone could still compile the code with an
  older compiler.
 
 Qt 5 has Q_NULLPTR that expands to nullptr on C++11 and 0 otherwise.

Is there any chance that these macros will find their way into Qt4? I don't 
mean that they should be used by Qt itself everywhere (or at all), but 
providing them would allow an easy forward-compatible way of introducing this 
stuff. That may not really help for KDE (or we need to require 4.8.3 or need to 
ship them ourself) but for other stuff out there this would be an improvement 
that you basically get for free.

Eike

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

Re: Compiler version

[Rolf Eike Beer]

 Hi all,

 I've tested the waters some time ago [1] what would people say if we
 started asking for more modern compilers. I've stated there I'll start
 the discussion on k-c-d once we branch out 4.9, so I'm doing as
 promised. The post was only about kactivities, but the same could be
 applied to more stuff.

That reminds me of a question I always wanted to ask: is there any reason
why we don't use '#pragma once', i.e. is there a supported compiler that
can't handle this?

 Mainly, the responses were positive (from both users and developers).

What is the current minimum requirement?

 As an additional argument for raising the bar, here are the GCC
 versions in most modern distros (collected by other people, didn't
 check):
 - Debian - 4.7 (testing)
 - openSuse 12.1 - 4.6

openSUSE 11.4 - 4.5
openSUSE 12.2 (upcoming) - 4.7

Eike

Re: Review Request: KJS: Implement Object.GetOwnPropertyDescriptor Object.DefineProperty

[Rolf Eike Beer]

 On April 29, 2012, 6:26 p.m., Maks Orlovich wrote:
  kjs/object.cpp, line 433
  http://git.reviewboard.kde.org/r/104515/diff/4/?file=58277#file58277line433
 
  more * inconsistency
 
 Bernd Buschinski wrote:
 I don't get the problem here, could you please explain?

GetterSetterImp *gs - GetterSetterImp* gs


- Rolf Eike


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104515/#review13085
---


On June 1, 2012, 12:34 p.m., Bernd Buschinski wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/104515/
 ---
 
 (Updated June 1, 2012, 12:34 p.m.)
 
 
 Review request for kdelibs.
 
 
 Description
 ---
 
 KJS: Implement Object.GetOwnPropertyDescriptor  Object.DefineProperty
 
 This is a pretty big patch, to get Object.defineProperty perfect for 
 ecmascript (for all tests that only use implemented stuff, all test that use 
 Object.create for example will fail, as its not implemented)
 
 PropertyDescriptor:
 Necessary for collectiong data, this introduce new CommonIdentifiers.h, this 
 might requiere to rebuild khtml against new kjs, otherwise it might cause 
 weird crashes (at least for me)
 
 
 object.h:
 Beside from adding new getPropertyDescriptor  getOwnPropertyDescriptor  
 defineOwnProperty, the important changes are making
 getPropertyAttributes, put/get/remove-Direct virtual.
 Why do I need that?
 Because put checks if the prototype already has property XYZ and uses it. Now 
 imagine an array that got a setter-only property via a prototype. 
 DefineProperty would try to use put, which uses the prototype property and it 
 would fail. So all custom-data classes like Array need to implement/use 
 put/get/remove-Direct.
 
 
 object.cpp:
 currently put on a setter-only property would always throw an exception, this 
 is only correct for strict-mode, as we currently do not check for strict-mode 
 it would make more sense to change it to default not throwing an exception.
 
 
 array.cpp/h:
 The old Array implementation did not store attributes for array indexes, I 
 rewrote it to also store the attributes.
 + Bonus: also fix getOwnPropertyNames, as we now store attributes.
 + use new attributes, reject put/delete/enum if set
 
 function.cpp (Arguments)
 changed the default attributes how parameter are stored, according to ECMA 
 10.6.11.b
 
 
 Rest is just the defineProperty implementation
 
 
 Diffs
 -
 
   kjs/CMakeLists.txt 1188064 
   kjs/CommonIdentifiers.h 8ee40e8 
   kjs/array_instance.h 3f2b630 
   kjs/array_instance.cpp fe9b8b4 
   kjs/function.h 3757fe8 
   kjs/function.cpp 5f39ae6 
   kjs/object.h 047c242 
   kjs/object.cpp c19122f 
   kjs/object_object.cpp 986f03f 
   kjs/operations.h f8a28c8 
   kjs/operations.cpp d4c0066 
   kjs/propertydescriptor.h PRE-CREATION 
   kjs/propertydescriptor.cpp PRE-CREATION 
 
 Diff: http://git.reviewboard.kde.org/r/104515/diff/
 
 
 Testing
 ---
 
 ecmascript  daily surfing
 
 used valgrind on many array testcases to check for possible memleaks
 
 
 Thanks,
 
 Bernd Buschinski
 

Re: Review Request: kjs: Implement JSON.parse

[Rolf Eike Beer]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105056/#review14177
---


You forgot to change kjs/tests/ecmatest_broken_*

- Rolf Eike Beer


On May 25, 2012, 8:19 p.m., Bernd Buschinski wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/105056/
 ---
 
 (Updated May 25, 2012, 8:19 p.m.)
 
 
 Review request for kdelibs.
 
 
 Description
 ---
 
 kjs: Implement JSON.parse
 
 
 Diffs
 -
 
   kjs/CMakeLists.txt 1188064 
   kjs/interpreter.cpp cf1acf1 
   kjs/json_object.h PRE-CREATION 
   kjs/json_object.cpp PRE-CREATION 
   kjs/jsonlexer.h PRE-CREATION 
   kjs/jsonlexer.cpp PRE-CREATION 
   kjs/libkjs.map e9f679f 
 
 Diff: http://git.reviewboard.kde.org/r/105056/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Bernd Buschinski
 

Re: Review Request: kjs: Implement JSON.stringify

[Rolf Eike Beer]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105057/#review14178
---


You forgot to change kjs/tests/ecmatest_broken_*


kjs/json_object.cpp
http://git.reviewboard.kde.org/r/105057/#comment11211

Duplicate newline


- Rolf Eike Beer


On May 25, 2012, 8:19 p.m., Bernd Buschinski wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/105057/
 ---
 
 (Updated May 25, 2012, 8:19 p.m.)
 
 
 Review request for kdelibs.
 
 
 Description
 ---
 
 kjs: Implement JSON.stringify
 
 patch needs https://git.reviewboard.kde.org/r/105056/ (JSON.parse)
 
 
 Diffs
 -
 
   kjs/CMakeLists.txt 1188064 
   kjs/CommonIdentifiers.h 8ee40e8 
   kjs/json_object.h PRE-CREATION 
   kjs/json_object.cpp PRE-CREATION 
   kjs/jsonstringify.h PRE-CREATION 
   kjs/jsonstringify.cpp PRE-CREATION 
 
 Diff: http://git.reviewboard.kde.org/r/105057/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Bernd Buschinski
 

Re: KDELibsNightly.cmake

[Rolf Eike Beer]

Am Sonntag, 20. Mai 2012, 12:55:54 schrieb Volker Krause:
 On Sunday 20 May 2012 10:08:11 Andreas Pakulat wrote:

  Anyway, I guess the guys ultimately knowning how this is done for kdelibs
  are Alex or Volker...
 
 I'm not using those scripts either. My nightly build just calls ctest
 directly, similar to what you have done. Instead of the -D shortcut I use
 -M/- T though, to also enable the coverage upload and valgrinded test run
 steps.

CTest on what? An already build tree? How do you get the build results then? 
Or which CTest script do you use? Would you mind updating that techbase page 
on how that works?

Eike

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

Re: KDELibsNightly.cmake

[Rolf Eike Beer]

Am Donnerstag, 17. Mai 2012, 13:30:24 schrieb Andreas Pakulat:
 Hi,
 
 I think this techbase article is pretty much up-to-date. I guess the
 KDElibsNighly.cmake file should simply be deleted.
 http://techbase.kde.org/Development/CMake/DashboardBuilds

This points to quality/nightly-support/KDE/KDELibsNightly.cmake:

# The VCS of KDE is svn, also specify the repository
set(KDE_CTEST_VCS svn)
set(KDE_CTEST_VCS_REPOSITORY svn://anonsvn.kde.org/home/kde/trunk/KDE/kdelibs)

Eike

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

Review Request: KJS: improve regex flag scanning

[Rolf Eike Beer]

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

Review request for kdelibs.


Description
---

Only scan the list of flags once, and only if there is a list at all.

It also fixes the S15.10.4.1_A5_T1 and S15.10.4.1_A5_T1 tests of ECMA test262:
a regex flag may be given only once.

I also checked Firefox behaviour, it also throws a syntax error in this case.


Diffs
-

  kjs/regexp_object.cpp a6b6974 

Diff: http://git.reviewboard.kde.org/r/104974/diff/


Testing
---

Run ECMA testsuite. These 2 tests now pass, no other changes.


Thanks,

Rolf Eike Beer

Review Request: KJS: fix behaviour on allocation errors

[Rolf Eike Beer]

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

Review request for kdelibs.


Description
---

The KJS allocator will likely crash with a 0-deref on allocation errors. The 
exact behaviour will also depend on the platform, e.g. a Un*x platform without 
posix_memalign() will have MAP_FAILED as the pointer used for calculations 
(which is (void*)-1), other will have 0.

This will make the allocator have a sane default behaviour: just return 0.


Diffs
-

  kjs/collector.cpp 70e4757 

Diff: http://git.reviewboard.kde.org/r/104960/diff/


Testing
---


Thanks,

Rolf Eike Beer

Re: Review Request: dataprotocol: make charset recoding work

[Rolf Eike Beer]

 On May 9, 2012, 12:04 p.m., David Faure wrote:
  kio/kio/dataprotocol.cpp, line 71
  http://git.reviewboard.kde.org/r/104874/diff/1/?file=63068#file63068line71
 
  Not sure the comment is still correct. decoded? It's not, anymore, 
  right? It's just extracted? Or is it even the full initial URL?

Yes, it is still correct. This is after the percent encoding has been decoded.


 On May 9, 2012, 12:04 p.m., David Faure wrote:
  kio/kio/dataprotocol.cpp, line 277
  http://git.reviewboard.kde.org/r/104874/diff/1/?file=63068#file63068line277
 
  Why .toUtf8()? Are we sure that this is what the receiver of the data 
  will use, for decoding?
  (I mean in the real case of an application getting the data, not in the 
  unittest)

I have used the new testcases on the old code and compared with the online 
tests. The only cases where old code was right and the string afterwards was 
displayed correctly was when it returned UTF8 strings. So returning UTF8 is the 
right thing here IMHO.


 On May 9, 2012, 12:04 p.m., David Faure wrote:
  kio/kio/dataprotocol.cpp, line 279
  http://git.reviewboard.kde.org/r/104874/diff/1/?file=63068#file63068line279
 
  This comment isn't applicable anymore, it was the justification for 
  toLocal8Bit().

Correct, will delete that.


- Rolf Eike


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104874/#review13603
---


On May 6, 2012, 6:14 p.m., Rolf Eike Beer wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/104874/
 ---
 
 (Updated May 6, 2012, 6:14 p.m.)
 
 
 Review request for kdelibs.
 
 
 Description
 ---
 
 This reworks the code that works with different character sets to actually do 
 the right thing (tm).
 
 
 Diffs
 -
 
   kio/kio/dataprotocol.cpp e614476 
   kio/tests/dataprotocoltest.cpp c8df132 
 
 Diff: http://git.reviewboard.kde.org/r/104874/diff/
 
 
 Testing
 ---
 
 -build whole kdelibs
 -added more testcases from http://greenbytes.de/tech/tc/datauri
 -all dataprotocol tests pass
 
 
 Thanks,
 
 Rolf Eike Beer
 

Re: Review Request: dataprotocol: simplify helper code

[Rolf Eike Beer]

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

(Updated May 6, 2012, 6:10 p.m.)


Review request for kdelibs.


Changes
---

Rebased diff against current KDE/4.8 branch.


Description
---

-add some const and static
-remove function parameters that always have the same values, use local statics
 in the function to hold these
-QChar(QLatin1Char('\0')) = QChar()
-QChar == QLatin1Char('\0') = QChar::isNull()


Diffs (updated)
-

  kio/kio/dataprotocol.cpp e614476 

Diff: http://git.reviewboard.kde.org/r/104860/diff/


Testing
---

-build whole kdelibs
-dataprotocol testcases still pass


Thanks,

Rolf Eike Beer

Re: What about DATAKIOSLAVE?

[Rolf Eike Beer]

Am Samstag, 5. Mai 2012, 12:16:58 schrieb Leo Savernik:
 Am Freitag, 4. Mai 2012 18:53 schrieb Rolf Eike Beer:
  In kio/kio/dataprotocol.h you will find this documentation of that
  #define:
  
  // DATAKIOSLAVE: define if you want to compile this into a stand-alone
  //  kioslave
  
  And in this header and the implementation a bunch of #ifdef's depending on
  it. But I can't see it being set or used anywhere. Would anyone object on
  just killing it altogether?
 
 That's simple, you can compile it into a standalone executable that is
 registered like any other kioslave.

Yes, that what I guessed.

 However, given that data-urls don't depend on any external data it seemed
 overkill, therefore I added a special hack that the kio-dataslave is invoked
 in-process on the client side.
 
 Hence, by defining DATAKIOSLAVE you can disable this special hack and
 compile dataprotocol.* into a standalone kioslave.

So what we currently have is the hack, with this define one would get the 
normal behaviour.

 Which bug or breakage do you fix by removing DATAKIOSLAVE?

I was just wondering what that stuff is all about since it seems unused. Maybe 
you could just write this down in a comment?

Eike

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

What about DATAKIOSLAVE?

[Rolf Eike Beer]

In kio/kio/dataprotocol.h you will find this documentation of that #define:

// DATAKIOSLAVE: define if you want to compile this into a stand-alone
//  kioslave

And in this header and the implementation a bunch of #ifdef's depending on
it. But I can't see it being set or used anywhere. Would anyone object on
just killing it altogether?

Eike

Re: Review Request: data: protocol: don't treat fragments as part of the data

[Rolf Eike Beer]

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

(Updated April 26, 2012, 4:31 p.m.)


Review request for kdelibs.


Changes
---

Incorporated the comments from David Faure.


Description
---

The testcases at http://greenbytes.de/tech/tc/datauri/ show that we incorrectly 
take the fragment (that's everything starting with the #) in a link containing 
a data URL as part of the URL. Fix it ;)


Diffs (updated)
-

  kio/kio/dataprotocol.cpp ea95de4 
  kio/tests/dataprotocoltest.cpp 44410de 

Diff: http://git.reviewboard.kde.org/r/104624/diff/


Testing
---

Old unittests still pass, new ones now also.


Thanks,

Rolf Eike Beer

Re: Moving libsolid-hal to unmaintained?

[Rolf Eike Beer]

 Is there a way to have it so HAL is enabled by default on BSD systems,
 but on Linux systems you need to manually use a cmake flag to enable
 it?

if (CMAKE_SYSTEM_NAME MATCHES BSD)
  set(HAL_OPTION_DEFAULT TRUE)
else ()
  set(HAL_OPTION DEFAULT FALSE)
endif ()

option(KDE4_USE_HAL Enable Solid backend using HAL ${HAL_OPTION_DEFAULT})

Eike

Re: Review Request: HTTP ioslave: do not require incoming headers to have spaces after colon

[Rolf Eike Beer]

 On April 16, 2012, 11:34 p.m., Dawit Alemayehu wrote:
  But your change now makes it possible to have a header without any spaces 
  after the colon as well. Despite that only affecting headers from the 
  cache, which what your patch does, I do not see the point here. Do you want 
  to support cases where there is no space between the ':' and the header 
  content ?

Sorry, that sentence from the description got lost somehow:

While we already support any as = 1, spaces or tabs by calling trimmed() 
before checking we have been failing for any as in nothing.

So: yes, this is the whole purpose of this patch.


- Rolf Eike


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104625/#review12545
---


On April 16, 2012, 8:30 p.m., Rolf Eike Beer wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/104625/
 ---
 
 (Updated April 16, 2012, 8:30 p.m.)
 
 
 Review request for kdelibs.
 
 
 Description
 ---
 
 The RfCs suggest to use a single space after the colon in HTTP headers, but 
 any amount of whitespace is allowed. This has been fixed for 
 Content-Disposition headers a while ago. Now also fix it for the other 
 headers we explicitely check here.
 
 
 Diffs
 -
 
   kioslave/http/http.cpp f7aa857 
 
 Diff: http://git.reviewboard.kde.org/r/104625/diff/
 
 
 Testing
 ---
 
 Still compiles and I see no breakage.
 
 
 Thanks,
 
 Rolf Eike Beer
 

Re: Review Request: data: protocol: don't treat fragments as part of the data

[Rolf Eike Beer]

 On April 17, 2012, 7:47 p.m., David Faure wrote:
  kio/kio/dataprotocol.cpp, line 181
  http://git.reviewboard.kde.org/r/104624/diff/1/?file=56827#file56827line181
 
  This looks wrong. If you call url.path(), you have a decoded string, so 
  no need to call fromPercentEncoding on it.

You are right. I removed the fromPercentEncoding() and everything is fine.


 On April 17, 2012, 7:47 p.m., David Faure wrote:
  kio/kio/dataprotocol.cpp, line 189
  http://git.reviewboard.kde.org/r/104624/diff/1/?file=56827#file56827line189
 
  If data_offset is now always 0, this if can never pass, can it? Or 
  maybe if raw_url_len is 0, but in any case this could be simplified to if 
  (raw_url_len == 0)...

Yes, changed to (raw_url_len == 0)


- Rolf Eike


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104624/#review12612
---


On April 16, 2012, 8:26 p.m., Rolf Eike Beer wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/104624/
 ---
 
 (Updated April 16, 2012, 8:26 p.m.)
 
 
 Review request for kdelibs.
 
 
 Description
 ---
 
 The testcases at http://greenbytes.de/tech/tc/datauri/ show that we 
 incorrectly take the fragment (that's everything starting with the #) in a 
 link containing a data URL as part of the URL. Fix it ;)
 
 
 Diffs
 -
 
   kio/kio/dataprotocol.cpp ea95de4 
   kio/tests/dataprotocoltest.cpp 6d7f1c7 
 
 Diff: http://git.reviewboard.kde.org/r/104624/diff/
 
 
 Testing
 ---
 
 Old unittests still pass, new ones now also.
 
 
 Thanks,
 
 Rolf Eike Beer
 

Review Request: HTTP ioslave: do not require incoming headers to have spaces after colon

[Rolf Eike Beer]

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

Review request for kdelibs.


Description
---

The RfCs suggest to use a single space after the colon in HTTP headers, but any 
amount of whitespace is allowed. This has been fixed for Content-Disposition 
headers a while ago. Now also fix it for the other headers we explicitely check 
here.


Diffs
-

  kioslave/http/http.cpp f7aa857 

Diff: http://git.reviewboard.kde.org/r/104625/diff/


Testing
---

Still compiles and I see no breakage.


Thanks,

Rolf Eike Beer

Re: Hunting a memory leak in KMail

[Rolf Eike Beer]

Am Samstag 31 März 2012, 23:48:57 schrieb Alex Fiestas:
 For some time I have been experiencing a memory leak that makes KMail go up
 to 1,5Gb of ram when using it for a long period of time, Volker saw it with
 his own eyes and we even tried to valgrind it.

 I have been using KMail while keeping an eye in ksysguard and I think I know
 where the leak is (or at least one of them).

 Each time I click in an email, memory is leaked, not much but it can be
 appreaciated if you do:

 0-Open system activity and take a look at KMail's RAM
 1-Enter into a folder with some emails (more than 100?)
 2-Click on one email
 3-Long press N (Next email)
 4-See how memory increase

 Changing between folders (I know KMails cache some data) doesn't reduce the
 amount of memory used.

 Can anyone reproduce this?

Yes, I can. And it doesn't look as if you need many mail. Looks like when the
last mail in the folder is hit n jumps to the first one again, so I cycled a
few dozen times through the same 10 mails and the memory usage increased about
10M.

Eike

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

Re: Review Request: Fix FindXine.cmake to use pkg-config instead of xine-config

[Rolf Eike Beer]

 - remove the if(WIN32)
 around the pkgconfig-stuff. People say it should work also on Windows. So
 let's see.

But add an if() to check if pkg-config is actually found?

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

Re: Review Request: Fix FindXine.cmake to use pkg-config instead of xine-config

[Rolf Eike Beer]

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

 Review request for kdelibs.

If it is a CMake-related issue it's always a wise idea to explicitely add
Alex Neundorf as he is by default guilty of all CMake bugs ;) Alex,
explicitely added you to CC now. Are you on k-c-d?

 Description
 ---

 This is a gentoo downstream patch, see bug
 https://bugs.gentoo.org/show_bug.cgi?id=397595 for cause.

While this patch may just work, especially for 1.2, I don't like the way
it is done. The first question is: does the pkg-config approach also work
with 1.1.0 (as that is the minimum required version)?

And if yes: then please use the FindPkgConfig module instead of open
coding such things.

Eike

Re: Review Request: Fix Date Display in dolphin info panel

[Rolf Eike Beer]

 On Dec. 1, 2011, 8:40 a.m., Sebastian Trueg wrote:
  Ship It!

 Seems I can't commit to kdelibs, could you commit this? Thank you.

If you have a developer account you can commit there. Maybe you just tried
to commit to the locked master branch? Try committing to KDE/4.7.

Eike

Re: Review Request: Pretty resize of RenameDialog according to its content

[Rolf Eike Beer]

 Several small fixes to allow RenameDialog form and its widgets resize
 itself according to extra information gathered for source and destination
 items.

 Without these fixes RenameDialog in common case appears as shown on
 screenshot 1 (without preview for files) and 2 (without preview for
 files). After the fixes it looks like on screenshot 3 and 4. Screen size
 is respected correctly, if space doesn't allow, scrollbars appears in
 scroll areas as before.

Yes, please. This is how this dialog should look like. Awesome.

 Quite thoroughly tested in case of preview and no preview, all the Qt
 layout issues are tested and seem to be OK.

One could say the current behaviour is not only ugly, but a bug. Then this
could go into 4.7.

+1 from me

Eike

Re: Review Request: kio_http: fix keepalive timeout parsing

[Rolf Eike Beer]

 Testing
 ---

 -Patched code compiles
 -Hacked a web server and made tests against following keep-alive header
 variants:
  Keep-Alive: timeout=5, max=99
  Keep-Alive: Timeout=5, max=99 (uppercase 'T')
  Keep-Alive: Timeout=5 , max=99(extra space before comma)

I don't know which RfC this comes from and if that makes any requirements
about the ordering, but other header fields (like Content-Disposition) do
not have some. So what happens with e.g.:

max=99, timeout=5
max = 99, timeout = 5
foo = bar, timeout = 5

Is it possible to add a unit test for this?

Eike

Re: The case for a kdelibs 4.8

[Rolf Eike Beer]

 For example, when we switched our default
 spell checker in Fedora from aspell to hunspell in Fedora 9 (i.e. 4.0
 era), I
 had to add support for hunspell to kdelibs3, or our users would have had
 to
 install 2 spell checkers to use KDE apps! (Even several apps in the
 default
 KDE installation hadn't been ported to kdelibs4 yet in Fedora 9.) That
 change
 never got upstreamed because kdelibs3 is no longer maintained, yet it
 would
 have been useful to many distributions. Please keep the life cycle of your
 software for distributors and end users in mind when you decide your
 schedules, not just the convenience of a few core developers.

Given that it is now proven and tested code, who stops you committing it
into KDE/3.5 branch?

 4. The core developers, for whose convenience this change was made, are
 not the only ones working or willing to work on kdelibs. The main reason
 that was given for dropping kdelibs 4.8 is that this means one less branch
 to worry about for the people working on kdelibs, but the people who'd
 work on 4.8 and 5.0 are NOT the same people! I understand that the
 developers who are pushing forward towards the new frameworks are not
 interested in 4.8, but you should understand that many other KDE
 contributors are not interested at all in 5.0 at this time. I, for one,
 will start caring about 5.0 the day some application (be it a Plasma
 workspace or a regular application) using it will enter Rawhide (Fedora's
 trunk), and I guess other distro folks are feeling the same.
 OTOH, I'm very much interested in working on 4.8, and in fact I already
 did (see my Plasma PackageKit GSoC work), but my patches are now stuck in
 reviewboard and cannot be committed due to the deep freeze. Do you really
 want to encourage wild patching by distributions, adding or backporting a
 patchwork (pun intended) of features? I remember Aaron Seigo complaining
 on his blog about the feature backports done by distributions in 4.0, 4.1
 and 4.2 era, but if you aren't going to allow any new features upstream,
 you will be leaving us no choice.

The reason to stop master was (as far as I understand) to make the
frameworks branch easily to maintain. If someone is working on 4.8
(bugfixing, features) all this has to be ported to frameworks, too. So you
develop a moving target on a moving target.

Not that I do not understand your point (I'm not sure if I have an opinion
on this yet), just to get a common understanding about the reasons.

I would have wished more than once if we could have done a new release on
an older branch, e.g. a KDE 4.6.6 where the KLineEdits in Konqueror would
be fixed. Some sort of long-term branch, or at least one maintenance
release more when the .0 or .1 is out. You only get severe end user
testing on the new major release when the .0 is out, so those who don't
want to break their system will stay on the older branch, which has bugs
long fixed in the new one. Some sort of chicken and egg problem, of
course, but it would be helpful for those people to get an additional
release with bugfixes after the next .0 is out. This could be e.g. in the
middle between .0 and .1 to not have the release workload for 2 releases
at once. And: yes, I know, this is even more work for the release team.
Poor Dirk.

Eike

Re: Review Request: little faster sycoca

[Rolf Eike Beer]

Am Mittwoch, 28. September 2011, 15:47:25 schrieb Josef Weidendorfer:
 On Wednesday 28 September 2011, Jaime Torres Amate wrote:

  and the removal of a for loop (I'm checking it this has been this way
  since the beginning, or if fixing it makes other things faster) as Rolf
  has pointed.
 
 I do not see how that loop can be removed. He probably misread the inner
 condition of the first loop as if (pos  1 , but it is actually
 if (pos  entry.length, and pos can be any position in the string.

I meant this:

if (inPos  0) {
pos = -inPos-1;
for(KSycocaDictStringList::const_iterator it = stringlist-
constBegin(); it != stringlist-constEnd(); ++it)
{
string_entry* entry = *it;
register int l = entry-length;
if (pos  l  pos != 0) {
...
}
}

What happens if inPos is -1? pos becomes 0 then. Then we iterate over the 
whole list just to do if (...  pos != 0) which will never be true. So for 
this case (inPos == -1) the whole function can be avoided at all as it will 
never return anything else but 0. So the initial check of the function should 
IMHO be:

if (inPos == 0 || inPos == -1)
return 0;

Eike

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

Re: Review Request: little faster sycoca

[Rolf Eike Beer]

Am Dienstag, 27. September 2011, 20:42:54 schrieb Jaime Torres Amate:

 before: 2984 calls to constBegin, 0,00%. 2960531 calls to constEnd, 2.33%
 after: 2921 calls to constBegin, 0,00%. 2921 calls to constEnd, 0.00%
 
 before: calcDiversity, 55.83%  (debug enabled)
 after: calcDiversity, 14,46%  (debug enabled)
 
 buildsycoca is still not faster than light, but it is only a four lines
 patch.

From a quick view: looks good.

The trunkcate(sz) isn't needed AFAICT as matrix' size is never changed after 
the constructor so it already has this size.

Ok, let's look on some other points:

-the register inside the inner if's is superfluous, there is no place a 
function result could be stored between two function calls but in a register

-from what I see the first branch iterates through the loop if pos is -1 and 
never does anything. -(-1)-1 is 0, and inside the loop it checks pos != 0. 
Since pos can't change inside the loop it should be checked outside the loop 
so the whole iterating can be avoided at all. Or it could be merged with the 
first line of the function to immediately return 0.

-the remaining register statement now is probably useless, too. The value is 
only preserved for very few instructions, so the compiler should already do 
this right. Hopefully.

Eike

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

Re: Review Request: faster kmd5

[Rolf Eike Beer]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102165/#review5284
---



kdecore/text/kcodecs.cpp
http://git.reviewboard.kde.org/r/102165/#comment4785

Please remove this register keyword. In best case the compiler moves this 
value into a register where he had placed it anyway. But usually you force a 
value into a register that the compiler could use for something better.


- Rolf Eike


On July 31, 2011, 5:28 p.m., Jaime Torres Amate wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/102165/
 ---
 
 (Updated July 31, 2011, 5:28 p.m.)
 
 
 Review request for kdelibs.
 
 
 Summary
 ---
 
 I think the private methods can be changed without chaning the binary 
 interface..
 In that case, I've changed some kmd5 methods to make them const, and pass 
 const numbers.
 Also, in the transform method I've added register to the a,b,c,d variables, 
 changing the FF, GG, HH ad II to return qint32 (hopefully in a register also).
 
 In any case, the memset at the end of transform can be avoided. 
 There is also a little change to not declare and inicialize and never use a 
 variable in quotedprintable.
 
 
 Diffs
 -
 
   kdecore/text/kcodecs.h 023a129 
   kdecore/text/kcodecs.cpp ce0e48d 
 
 Diff: http://git.reviewboard.kde.org/r/102165/diff
 
 
 Testing
 ---
 
 Running kphotoalbum maintenance|Recalculate checksum, that makes extensive 
 use of kmd5, under callgrind, it shows that the new version is faster.
 At least in an AMD64.
 In the old way, FF, GG, HH and II calls in the transform method took 1,10%; 
 1,07%; 1,00%; 1,03%  +0,33% passing parameters
 In the new way, FF, GG, HH and II calls in the transform method take 0,83%; 
 0,80%; 0,76%; 0,78%  +0,21% passing parameters
 
 I've tried also the kmd5benchmarktest, but I do not understand very well the 
 results (or if they are as accurate as callgrind).
 
 
 Thanks,
 
 Jaime Torres