D16832: fix empty runner list after switching to new locale

[Nick Shaforostoff]

shaforostoff added inline comments.

INLINE COMMENTS

> davidedmundson wrote in runnermanager.cpp:274
> what does deadRunners have to do with this?

if deadRunners is empty then there is no sense rerunning this method, and it 
even could cause endless recursion

REPOSITORY
  R308 KRunner

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

To: shaforostoff, #plasma_workspaces, davidedmundson, #frameworks
Cc: kde-frameworks-devel, michaldybczak, sdepiets, michaelh, ngraham, bruns, 
skadinna, huftis

Re: Review Request 127896: make dbus optional on osx: kauth

[Nick Shaforostoff]

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

(Updated May 20, 2016, 9:13 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks, David Edmundson, 
and Martin Gräßlin.


Changes
---

made dbus enabling/disabling be available via cmake config option only, as asked


Repository: kauth


Description
---

this is the first patch to make kde frameworks build (and then work) without 
dbus.

this will allow homebrew users use precompiled vanilla Qt to build kde apps on 
osx. as dbus is not a common service in osx world, kde apps on osx should use 
native means for interprocess communication instead -- this will make them 
better citizens in osx ecosystem.


Diffs (updated)
-

  CMakeLists.txt 48dc2d9 
  autotests/BackendsManager.cpp 59675b3 
  autotests/CMakeLists.txt b53d760 
  autotests/HelperTest.cpp 8050a06 
  src/CMakeLists.txt 1b6930d 
  src/ConfigureChecks.cmake d46761a 

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


Testing
---

compiles fine on osx, compiles fine on linux, tests on linux still pass.


Thanks,

Nick Shaforostoff

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

Re: Review Request 127896: make dbus optional on osx: kauth

[Nick Shaforostoff]

> On May 12, 2016, 6:15 a.m., Kåre Särs wrote:
> > I think this patch should not include any platform specific defines. 
> > Disabling DBus requirement on Windows might also be interesting for some 
> > projects. I propose to do something similar to what is done in kxmlgui to 
> > disable kglobalaccel. The default is to require kglobalaccel, but if you 
> > knowingly specify "-DFORCE_DISABLE_KGLOBALACCEL=1" kglobalaccel is not 
> > required or searched for.
> 
> René J.V. Bertin wrote:
> True, even if there's probably very little point in disabling DBus on a 
> standard Unix/Linux set-up.
> 
> But indeed, a platform-agnostic option can still be included in the 
> options that will be flipped by a platform-specific option like 
> `APPLE_STANDALONE_BUNDLE` I've proposed in a patch for Marble.
> 
> To come back to what I suggested earlier: even if there were to be a KF5 
> framework that encapsulates DBus or "something more platform native" there 
> would still be a use-case for using DBus through that framework even on OS X 
> (or MS Windows). Projects like HomeBrew, MacPorts and Cygwin don't exist to 
> replace the native desktop, but to complement it; to provide an easy way to 
> install and maintain FOSS and provide a context in which those applications 
> can run as much as possible as they do on their native platform. That 
> evidently includes DBus, but not just so that Gnome apps can talk with other 
> Gnome apps and KDE apps with other KDE apps. I don't have any stats on this, 
> but I'd expect that a good many of the users of such projects use them not so 
> much for software development per se, but as tools for their actual work 
> (often in science and/or engineering, in my impression). That might very well 
> include using Gnome/GTk apps alongside KDE applications, and in that case 
> they'd proba
 bly expect  the same kind of interoperability as those apps would have on the 
other platforms they might use.
> IOW, I don't think a distribution system that aims to provide the best 
> possible context for the applications it carries can get around using DBus.
> 
> But this is probably not the best place for an in-depth discussion around 
> underlying principles; that should probably be done on a ML (and ideally 
> include a DBus dev or two).
> 
> Nick Shaforostoff wrote:
> i don't see a reason behind introducing a special cmake option other than 
> code coverage (test dbus and dbus-free branches with the same qt): why one 
> should want to disable dbus on a system where he/she has Qt with dbus?
> 
> can you explain this to me?
> 
> regarding homebrew, i repeat: by default it installs a precompiled 
> version of Qt without dbus. if user wants dbus then he/she has to have 
> homebrew recompile whole Qt (takes about 1 hour). and what if kde app doesn't 
> need any IPC? that would just pollute his/her system with redundant stuff. 
> how many gtk-based apps require dbus to run on windows and osx?
> 
> Kåre Särs wrote:
> We have Kate as an example. At the moment the main thing we need DBus for 
> on windows is opening documents in only one window when opening new documents 
> through explorer. Without the DBus daemon it does not work. KDevelop has a 
> solution for it without using DBus which means that we could skip DBus usage 
> for this purpose and we would not need to tell people to install the service 
> (DBus) that occasionally has been flagged as mallware.
> 
> You might argue that we should just compile support without installing 
> the service, but how do I know what features don't work without the service? 
> If the frameworks need special options to disable DBus I'm informed about 
> what is disabled and can choose if I need it or not. Without the option the 
> feature is silently disabled. This is the reason I want separate options for 
> each framework that provides it and not a global one in ECM.
> 
> Almost all Qt users on Windows that I know of would not even dream of 
> compiling their own Qt and pre-compiled Qt comes with DBus support.
> 
> René J.V. Bertin wrote:
> > can you explain this to me?
> 
> An option here would allow using a single Qt install to build and bundle 
> software either with or without support for DBus. 
> 
> I wouldn't worry about the "pollution" aspect of redundant stuff here. 
> There are other things that are just as redundant and pollute a lot more 
> (like "tons of" translation files) and that are still installed because disk 
> space is cheap compared to the potential cost of being overly concerned about 
> the ov

Re: Review Request 127896: make dbus optional on osx: kauth

[Nick Shaforostoff]

> On May 12, 2016, 5:43 a.m., René J.V. Bertin wrote:
> > CMakeLists.txt, lines 14-18
> > <https://git.reviewboard.kde.org/r/127896/diff/2/?file=464646#file464646line14>
> >
> > Am I right that on OS X use of DBus is going to depend on whether or 
> > not Qt provides the QtDBus component? If so, shouldn't this be done on MS 
> > Windows too?
> > 
> > If that's *not* the right interpretation:
> > PLEASE introduce proper option variables for this kind of thing, for 
> > instance in ECM. Consider the overal picture; is this only about DBus or is 
> > this ultimately about building standalone app bundles? In other words, 
> > would it be possible to name that option variable appropriately to allow it 
> > to act as a switch for other, related build options?
> > 
> > This is all the more appropriate (and *polite*) if this is a 
> > convenience change for HomeBrew - why would such a change oblige other 
> > packaging/distribution systems to add and maintain unknown amounts of 
> > additional patches to undo it?!

yes, on OS X use of DBus is going to depend on whether or not Qt provides the 
QtDBus component.


> On May 12, 2016, 5:43 a.m., René J.V. Bertin wrote:
> > autotests/BackendsManager.cpp, lines 26-30
> > <https://git.reviewboard.kde.org/r/127896/diff/2/?file=464647#file464647line26>
> >
> > Again, double-checking: Is QT_NO_DBUS really going to be defined if the 
> > cmake system didn't do a `find_package(Qt5 ... DBus)`? IOW, is this change 
> > not going to introduce a build failure on systems where Qt does provide a 
> > DBus interface?

i tested on linux - it did build fine


> On May 12, 2016, 5:43 a.m., René J.V. Bertin wrote:
> > autotests/BackendsManager.cpp, lines 56-60
> > <https://git.reviewboard.kde.org/r/127896/diff/2/?file=464647#file464647line56>
> >
> > Ditto, no risk of a build failure on systems where Qt does provide a 
> > DBus interface?

no rosk. please read the 'testing' section of this review request


> On May 12, 2016, 5:43 a.m., René J.V. Bertin wrote:
> > src/CMakeLists.txt, line 2
> > <https://git.reviewboard.kde.org/r/127896/diff/2/?file=464650#file464650line2>
> >
> > Was this a redundant statement?

yes, it was redundant.


- Nick


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


On May 12, 2016, 5:16 a.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127896/
> ---
> 
> (Updated May 12, 2016, 5:16 a.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks, David Edmundson, 
> and Martin Gräßlin.
> 
> 
> Repository: kauth
> 
> 
> Description
> ---
> 
> this is the first patch to make kde frameworks build (and then work) without 
> dbus.
> 
> this will allow homebrew users use precompiled vanilla Qt to build kde apps 
> on osx. as dbus is not a common service in osx world, kde apps on osx should 
> use native means for interprocess communication instead -- this will make 
> them better citizens in osx ecosystem.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 48dc2d9 
>   autotests/BackendsManager.cpp 59675b3 
>   autotests/CMakeLists.txt b53d760 
>   autotests/HelperTest.cpp 8050a06 
>   src/CMakeLists.txt 1b6930d 
>   src/ConfigureChecks.cmake d46761a 
> 
> Diff: https://git.reviewboard.kde.org/r/127896/diff/
> 
> 
> Testing
> ---
> 
> compiles fine on osx, compiles fine on linux, tests on linux still pass.
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

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

Re: Review Request 127896: make dbus optional on osx: kauth

[Nick Shaforostoff]

> On May 12, 2016, 6:15 a.m., Kåre Särs wrote:
> > I think this patch should not include any platform specific defines. 
> > Disabling DBus requirement on Windows might also be interesting for some 
> > projects. I propose to do something similar to what is done in kxmlgui to 
> > disable kglobalaccel. The default is to require kglobalaccel, but if you 
> > knowingly specify "-DFORCE_DISABLE_KGLOBALACCEL=1" kglobalaccel is not 
> > required or searched for.
> 
> René J.V. Bertin wrote:
> True, even if there's probably very little point in disabling DBus on a 
> standard Unix/Linux set-up.
> 
> But indeed, a platform-agnostic option can still be included in the 
> options that will be flipped by a platform-specific option like 
> `APPLE_STANDALONE_BUNDLE` I've proposed in a patch for Marble.
> 
> To come back to what I suggested earlier: even if there were to be a KF5 
> framework that encapsulates DBus or "something more platform native" there 
> would still be a use-case for using DBus through that framework even on OS X 
> (or MS Windows). Projects like HomeBrew, MacPorts and Cygwin don't exist to 
> replace the native desktop, but to complement it; to provide an easy way to 
> install and maintain FOSS and provide a context in which those applications 
> can run as much as possible as they do on their native platform. That 
> evidently includes DBus, but not just so that Gnome apps can talk with other 
> Gnome apps and KDE apps with other KDE apps. I don't have any stats on this, 
> but I'd expect that a good many of the users of such projects use them not so 
> much for software development per se, but as tools for their actual work 
> (often in science and/or engineering, in my impression). That might very well 
> include using Gnome/GTk apps alongside KDE applications, and in that case 
> they'd proba
 bly expect  the same kind of interoperability as those apps would have on the 
other platforms they might use.
> IOW, I don't think a distribution system that aims to provide the best 
> possible context for the applications it carries can get around using DBus.
> 
> But this is probably not the best place for an in-depth discussion around 
> underlying principles; that should probably be done on a ML (and ideally 
> include a DBus dev or two).

i don't see a reason behind introducing a special cmake option other than code 
coverage (test dbus and dbus-free branches with the same qt): why one should 
want to disable dbus on a system where he/she has Qt with dbus?

can you explain this to me?

regarding homebrew, i repeat: by default it installs a precompiled version of 
Qt without dbus. if user wants dbus then he/she has to have homebrew recompile 
whole Qt (takes about 1 hour). and what if kde app doesn't need any IPC? that 
would just pollute his/her system with redundant stuff. how many gtk-based apps 
require dbus to run on windows and osx?


- Nick


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


On May 12, 2016, 5:16 a.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127896/
> ---
> 
> (Updated May 12, 2016, 5:16 a.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks, David Edmundson, 
> and Martin Gräßlin.
> 
> 
> Repository: kauth
> 
> 
> Description
> ---
> 
> this is the first patch to make kde frameworks build (and then work) without 
> dbus.
> 
> this will allow homebrew users use precompiled vanilla Qt to build kde apps 
> on osx. as dbus is not a common service in osx world, kde apps on osx should 
> use native means for interprocess communication instead -- this will make 
> them better citizens in osx ecosystem.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 48dc2d9 
>   autotests/BackendsManager.cpp 59675b3 
>   autotests/CMakeLists.txt b53d760 
>   autotests/HelperTest.cpp 8050a06 
>   src/CMakeLists.txt 1b6930d 
>   src/ConfigureChecks.cmake d46761a 
> 
> Diff: https://git.reviewboard.kde.org/r/127896/diff/
> 
> 
> Testing
> ---
> 
> compiles fine on osx, compiles fine on linux, tests on linux still pass.
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

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

Re: Review Request 127896: make dbus optional on osx: kauth

[Nick Shaforostoff]

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

(Updated May 12, 2016, midnight)


Review request for KDE Frameworks, David Edmundson and Martin Gräßlin.


Changes
---

added KAUTH_HELPER_BACKEND_LIBS definition for compiling with 
-DKAUTH_HELPER_BACKEND_NAME=OSX cmake option


Repository: kauth


Description
---

this is the first patch to make kde frameworks build (and then work) without 
dbus.

this will allow homebrew users use precompiled vanilla Qt to build kde apps on 
osx. as dbus is not a common service in osx world, kde apps on osx should use 
native means for interprocess communication instead -- this will make them 
better citizens in osx ecosystem.


Diffs (updated)
-

  CMakeLists.txt 48dc2d9 
  autotests/BackendsManager.cpp 59675b3 
  autotests/CMakeLists.txt b53d760 
  autotests/HelperTest.cpp 8050a06 
  src/CMakeLists.txt 1b6930d 
  src/ConfigureChecks.cmake d46761a 

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


Testing
---

compiles fine on osx, compiles fine on linux, tests on linux still pass.


Thanks,

Nick Shaforostoff

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

Review Request 127896: make dbus optional on osx: kauth

[Nick Shaforostoff]

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

Review request for KDE Frameworks, David Edmundson and Martin Gräßlin.


Repository: kauth


Description
---

this is the first patch to make kde frameworks build (and then work) without 
dbus.

this will allow homebrew users use precompiled vanilla Qt to build kde apps on 
osx. as dbus is not a common service in osx world, kde apps on osx should use 
native means for interprocess communication instead -- this will make them 
better citizens in osx ecosystem.


Diffs
-

  CMakeLists.txt 05e2874 
  autotests/BackendsManager.cpp 59675b3 
  autotests/CMakeLists.txt b53d760 
  autotests/HelperTest.cpp 8050a06 
  src/CMakeLists.txt 1b6930d 

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


Testing
---

compiles fine on osx, compiles fine on linux, tests on linux still pass.


Thanks,

Nick Shaforostoff

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

dbus-less frameworks on osx

[Nick Shaforostoff]

hi all! I'm investigating possibility of getting some kde apps to build on osx 
with vanilla Qt without dbus.

would you accept patches that make that possible?

for example, i have just built kauth without dbus and i can prepare a review 
request with a (nice) patch for this
 
 
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127666: kconfig and kurlrequester friendship: store local paths in string-typed item without file:// prefix

[Nick Shaforostoff]

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

(Updated April 30, 2016, 11:45 p.m.)


Status
--

This change has been discarded.


Review request for KDE Frameworks and Matthew Dawson.


Repository: kconfig


Description
---

in ktorrent kcfg_*-names kurlrequester is used to edit a string-typed setting 
(it may contain only local paths).
this patch makes sure that no file:// prefix is added on setting change by 
adding a special check for the case when qvariant contains qurl and converting 
it appropriately


Diffs
-

  src/core/kcoreconfigskeleton.cpp 293387b 

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


Testing
---

ktorrent now displays local paths in settings dialog without file:// prefix


Thanks,

Nick Shaforostoff

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

Re[2]: Review Request 127632: Prioritize correct extension per theme

[Nick Shaforostoff]

sorry, I'm a bit late. I think having extensions without dots is preferred from 
common sense POV:

KDE-Extensions=svg
KDE-Extensions=svg,png

what do you think?

also, the spec would like additional params to start with X-, e.g.

X-KDE-Extensions=svg
X-KDE-Extensions=svg,png
 
 
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127632: Prioritize correct extension per theme

[Nick Shaforostoff]

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


Ship it!




i think adhering spec here is not very important.

we can adopt this change now, mention this somewhere in docs (print a qWarning 
saying that the checking order has changed?), and introduce X-KDE-Extensions 
later (which would only require a slght adjustment of iconPathByName() method).

- Nick Shaforostoff


On April 11, 2016, 11:17 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127632/
> ---
> 
> (Updated April 11, 2016, 11:17 p.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> Usually themes will have 1 kind of extension (2 tops) it doesn't make sense 
> to bindly use an extensions vector that is statically defined.
> 
> Prioritizes the extensions that work, so it will find the icons sooner.
> 
> 
> Diffs
> -
> 
>   src/kiconloader.cpp 37528ad 
>   src/kicontheme.h 3190665 
>   src/kicontheme.cpp e7e003b 
> 
> Diff: https://git.reviewboard.kde.org/r/127632/diff/
> 
> 
> Testing
> ---
> 
> Builds and tests still pass.
> Also reducess faulty disk accesses to its 45%. Note that by this metric, it's 
> almost as good as the RR #127236, but with a much smaller memory impact, 
> since we're not caching all the icon names (which was specially bad as it was 
> per-process).
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 4699
> $ strace kwrite |& grep ENOENT | wc -l
> 2119
> ```
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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

Review Request 127666: kconfig and kurlrequester friendship: store local paths in string-typed item without file:// prefix

[Nick Shaforostoff]

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

Review request for KDE Frameworks and Matthew Dawson.


Repository: kconfig


Description
---

in ktorrent kcfg_*-names kurlrequester is used to edit a string-typed setting 
(it may contain only local paths).
this patch makes sure that no file:// prefix is added on setting change by 
adding a special check for the case when qvariant contains qurl and converting 
it appropriately


Diffs
-

  src/core/kcoreconfigskeleton.cpp 293387b 

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


Testing
---

ktorrent now displays local paths in settings dialog without file:// prefix


Thanks,

Nick Shaforostoff

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

Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[Nick Shaforostoff]

> On March 1, 2016, 4:37 p.m., Aleix Pol Gonzalez wrote:
> > Eh... I just realized it's not 100% correct. We have a test 
> > (testUnknownIconNotCached) that fails, unsure how I didn't see it yesterday.
> > 
> > The problem with this one is that we are not reacting when icons are 
> > introduced at runtime. I tried adding a sneaky QFileSystemWatcher/KDirWatch 
> > on KIconTheme but it didn't signal.
> > 
> > Should I just discard the patch?
> 
> David Edmundson wrote:
> Could you compare modified time of mBaseDirThemeDir and reload if needed?
> it'd be one extra stat on the directory each time, but still a saving 
> from before
> 
> Aleix Pol Gonzalez wrote:
> That doesn't work, for some reason...
> ```
>  // Install the existing icon by copying.
> +qDebug() << "1" << 
> QFileInfo(actionIconsDir).lastModified();
>  QVERIFY(QFile::copy(QStringLiteral(":/test-22x22.png"), 
> newIconPath));
> +qDebug() << "2" << 
> QFileInfo(actionIconsDir).lastModified();
> ```
> 
> Both return the same value.
> 
> Nick Shaforostoff wrote:
> there is a difference between changing a file from inside the program and 
> changing it from outside.
> 
> did you make the change externally when testing QFileSystemWatcher-based 
> solution?
> 
> Nick Shaforostoff wrote:
> also for the QFileInfo::lastModified() there may be just not enough time 
> between the tests, so try adding a 2-minute sleep there
> 
> David Faure wrote:
> QFileInfo::lastModified() doesn't include milliseconds yet. There is a 
> pending patch for Qt to implement that (as part of a much bigger task which 
> also adds QFile::setFileTime()), but yeah, that's the problem. Right now you 
> need a 1s sleep whenever unittesting changes to lastModified().
> 
> QTest::qWait(1000); // remove this once lastModified includes ms
> 
> Or at least "up to 1s, until the end of the current second", I have code 
> for that in the kdirwatch unittest for instance.
> 
>     Another alternative, hacking the modification time on files (using utime, 
> like I do in ksycocatest.cpp)
> 
> (all this to avoid tests that take 15 seconds in total, I like the policy 
> that a test should run in under 5s -- otherwise people stop running them)
> 
> Nick Shaforostoff wrote:
> so, any news on this one?

even simple solution of discarding the cache after certain point of time (e.g. 
10 seconds) and going uncached code path would give us the benefit without 
introducing regression, right?


- Nick


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


On March 1, 2016, 3:25 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127236/
> ---
> 
> (Updated March 1, 2016, 3:25 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> At the moment we're playing Battleship to see if an icon is present in a 
> subdirectory. This means that we are checking on every directory if there's 
> an icon that matches the size with a said name on every request.
> 
> This can be seen easily with strace:
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
> ```
> After the patch: 
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
> ```
> We reduce these accesses to let QDir keep the list of files inside the 
> directory (that was already being generated at some point, it just was being 
> discarded).
> 
> 
> Diffs
> -
> 
>   src/kicontheme.cpp 0996054 
> 
> Diff: https://git.reviewboard.kde.org/r/127236/diff/
> 
> 
> Testing
> ---
> 
> Builds, tests still pass, applications start noticeably faster.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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

Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[Nick Shaforostoff]

> On March 1, 2016, 4:37 p.m., Aleix Pol Gonzalez wrote:
> > Eh... I just realized it's not 100% correct. We have a test 
> > (testUnknownIconNotCached) that fails, unsure how I didn't see it yesterday.
> > 
> > The problem with this one is that we are not reacting when icons are 
> > introduced at runtime. I tried adding a sneaky QFileSystemWatcher/KDirWatch 
> > on KIconTheme but it didn't signal.
> > 
> > Should I just discard the patch?
> 
> David Edmundson wrote:
> Could you compare modified time of mBaseDirThemeDir and reload if needed?
> it'd be one extra stat on the directory each time, but still a saving 
> from before
> 
> Aleix Pol Gonzalez wrote:
> That doesn't work, for some reason...
> ```
>  // Install the existing icon by copying.
> +qDebug() << "1" << 
> QFileInfo(actionIconsDir).lastModified();
>  QVERIFY(QFile::copy(QStringLiteral(":/test-22x22.png"), 
> newIconPath));
> +qDebug() << "2" << 
> QFileInfo(actionIconsDir).lastModified();
> ```
> 
> Both return the same value.
> 
> Nick Shaforostoff wrote:
> there is a difference between changing a file from inside the program and 
> changing it from outside.
> 
> did you make the change externally when testing QFileSystemWatcher-based 
> solution?
> 
> Nick Shaforostoff wrote:
> also for the QFileInfo::lastModified() there may be just not enough time 
> between the tests, so try adding a 2-minute sleep there
> 
> David Faure wrote:
> QFileInfo::lastModified() doesn't include milliseconds yet. There is a 
> pending patch for Qt to implement that (as part of a much bigger task which 
> also adds QFile::setFileTime()), but yeah, that's the problem. Right now you 
> need a 1s sleep whenever unittesting changes to lastModified().
> 
> QTest::qWait(1000); // remove this once lastModified includes ms
> 
> Or at least "up to 1s, until the end of the current second", I have code 
> for that in the kdirwatch unittest for instance.
> 
> Another alternative, hacking the modification time on files (using utime, 
> like I do in ksycocatest.cpp)
> 
> (all this to avoid tests that take 15 seconds in total, I like the policy 
> that a test should run in under 5s -- otherwise people stop running them)

so, any news on this one?


- Nick


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


On March 1, 2016, 3:25 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127236/
> ---
> 
> (Updated March 1, 2016, 3:25 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> At the moment we're playing Battleship to see if an icon is present in a 
> subdirectory. This means that we are checking on every directory if there's 
> an icon that matches the size with a said name on every request.
> 
> This can be seen easily with strace:
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
> ```
> After the patch: 
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
> ```
> We reduce these accesses to let QDir keep the list of files inside the 
> directory (that was already being generated at some point, it just was being 
> discarded).
> 
> 
> Diffs
> -
> 
>   src/kicontheme.cpp 0996054 
> 
> Diff: https://git.reviewboard.kde.org/r/127236/diff/
> 
> 
> Testing
> ---
> 
> Builds, tests still pass, applications start noticeably faster.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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

Re: Review Request 127215: simplify code, reduce pointer dereferences

[Nick Shaforostoff]

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

(Updated March 2, 2016, 12:03 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Changes
---

Submitted with commit b9c8b1fadcbb742e6129b77f28336c223a18b424 by Nick 
Shaforostoff to branch master.


Repository: kservice


Description
---

Changes are mostly related to containers/iterators, but there are also few 
QString related optimizations

note that i have made a switch from QLinkedList to just QList because it 
performs better for T=
also i have changed QStringList to QSet in one place to make the code 
run faster


Diffs
-

  src/services/kservice.cpp c75bce2 
  src/services/kservicetype.cpp 2a73ccd 
  src/sycoca/kbuildmimetypefactory.cpp 340f76a 
  src/sycoca/kbuildservicefactory.cpp a74f2e8 
  src/sycoca/kbuildsycoca.cpp e99e906 
  src/sycoca/kmimeassociations.cpp 9423b27 
  src/sycoca/ksycocadict.cpp f33d01a 
  src/sycoca/ksycocafactory.cpp e410581 
  src/sycoca/ksycocautils_p.h 8db26b0 

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


Testing
---

all tests pass, except kmimeassociationstest (fakeApplicationService is NULL), 
but it fails also without my changes


Thanks,

Nick Shaforostoff

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

Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[Nick Shaforostoff]

> On March 1, 2016, 4:37 p.m., Aleix Pol Gonzalez wrote:
> > Eh... I just realized it's not 100% correct. We have a test 
> > (testUnknownIconNotCached) that fails, unsure how I didn't see it yesterday.
> > 
> > The problem with this one is that we are not reacting when icons are 
> > introduced at runtime. I tried adding a sneaky QFileSystemWatcher/KDirWatch 
> > on KIconTheme but it didn't signal.
> > 
> > Should I just discard the patch?
> 
> David Edmundson wrote:
> Could you compare modified time of mBaseDirThemeDir and reload if needed?
> it'd be one extra stat on the directory each time, but still a saving 
> from before
> 
> Aleix Pol Gonzalez wrote:
> That doesn't work, for some reason...
> ```
>  // Install the existing icon by copying.
> +qDebug() << "1" << 
> QFileInfo(actionIconsDir).lastModified();
>  QVERIFY(QFile::copy(QStringLiteral(":/test-22x22.png"), 
> newIconPath));
> +qDebug() << "2" << 
> QFileInfo(actionIconsDir).lastModified();
> ```
> 
> Both return the same value.
> 
> Nick Shaforostoff wrote:
> there is a difference between changing a file from inside the program and 
> changing it from outside.
> 
> did you make the change externally when testing QFileSystemWatcher-based 
> solution?

also for the QFileInfo::lastModified() there may be just not enough time 
between the tests, so try adding a 2-minute sleep there


- Nick


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


On March 1, 2016, 3:25 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127236/
> ---
> 
> (Updated March 1, 2016, 3:25 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> At the moment we're playing Battleship to see if an icon is present in a 
> subdirectory. This means that we are checking on every directory if there's 
> an icon that matches the size with a said name on every request.
> 
> This can be seen easily with strace:
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
> ```
> After the patch: 
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
> ```
> We reduce these accesses to let QDir keep the list of files inside the 
> directory (that was already being generated at some point, it just was being 
> discarded).
> 
> 
> Diffs
> -
> 
>   src/kicontheme.cpp 0996054 
> 
> Diff: https://git.reviewboard.kde.org/r/127236/diff/
> 
> 
> Testing
> ---
> 
> Builds, tests still pass, applications start noticeably faster.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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

Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[Nick Shaforostoff]

> On March 1, 2016, 4:37 p.m., Aleix Pol Gonzalez wrote:
> > Eh... I just realized it's not 100% correct. We have a test 
> > (testUnknownIconNotCached) that fails, unsure how I didn't see it yesterday.
> > 
> > The problem with this one is that we are not reacting when icons are 
> > introduced at runtime. I tried adding a sneaky QFileSystemWatcher/KDirWatch 
> > on KIconTheme but it didn't signal.
> > 
> > Should I just discard the patch?
> 
> David Edmundson wrote:
> Could you compare modified time of mBaseDirThemeDir and reload if needed?
> it'd be one extra stat on the directory each time, but still a saving 
> from before
> 
> Aleix Pol Gonzalez wrote:
> That doesn't work, for some reason...
> ```
>  // Install the existing icon by copying.
> +qDebug() << "1" << 
> QFileInfo(actionIconsDir).lastModified();
>  QVERIFY(QFile::copy(QStringLiteral(":/test-22x22.png"), 
> newIconPath));
> +qDebug() << "2" << 
> QFileInfo(actionIconsDir).lastModified();
> ```
> 
> Both return the same value.

there is a difference between changing a file from inside the program and 
changing it from outside.

did you make the change externally when testing QFileSystemWatcher-based 
solution?


- Nick


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


On March 1, 2016, 3:25 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127236/
> ---
> 
> (Updated March 1, 2016, 3:25 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> At the moment we're playing Battleship to see if an icon is present in a 
> subdirectory. This means that we are checking on every directory if there's 
> an icon that matches the size with a said name on every request.
> 
> This can be seen easily with strace:
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
> ```
> After the patch: 
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
> ```
> We reduce these accesses to let QDir keep the list of files inside the 
> directory (that was already being generated at some point, it just was being 
> discarded).
> 
> 
> Diffs
> -
> 
>   src/kicontheme.cpp 0996054 
> 
> Diff: https://git.reviewboard.kde.org/r/127236/diff/
> 
> 
> Testing
> ---
> 
> Builds, tests still pass, applications start noticeably faster.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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

Re: Review Request 127215: simplify code, reduce pointer dereferences

[Nick Shaforostoff]

> On Feb. 29, 2016, 4:12 a.m., Aleix Pol Gonzalez wrote:
> > src/sycoca/ksycocadict.cpp, line 322
> > <https://git.reviewboard.kde.org/r/127215/diff/1/?file=445937#file445937line322>
> >
> > Maybe it's better to leave the * there, as it makes it more clear that 
> > it's an output argument.

well it's not really an output argument it is a 'processReplacing', the code 
only modifier the content pointed by the list, not the list itself (you see 
this by const_iterators used for walking through the list), so would everybody 
agree if i change the argument to 'const KSycocaDictStringList&'?


- Nick


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


On Feb. 29, 2016, 12:27 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127215/
> ---
> 
> (Updated Feb. 29, 2016, 12:27 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Changes are mostly related to containers/iterators, but there are also few 
> QString related optimizations
> 
> note that i have made a switch from QLinkedList to just QList because it 
> performs better for T=
> also i have changed QStringList to QSet in one place to make the 
> code run faster
> 
> 
> Diffs
> -
> 
>   src/services/kservice.cpp c75bce2 
>   src/services/kservicetype.cpp 2a73ccd 
>   src/sycoca/kbuildmimetypefactory.cpp 340f76a 
>   src/sycoca/kbuildservicefactory.cpp a74f2e8 
>   src/sycoca/kbuildsycoca.cpp e99e906 
>   src/sycoca/kmimeassociations.cpp 9423b27 
>   src/sycoca/ksycocadict.cpp f33d01a 
>   src/sycoca/ksycocafactory.cpp e410581 
>   src/sycoca/ksycocautils_p.h 8db26b0 
> 
> Diff: https://git.reviewboard.kde.org/r/127215/diff/
> 
> 
> Testing
> ---
> 
> all tests pass, except kmimeassociationstest (fakeApplicationService is 
> NULL), but it fails also without my changes
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

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

Re: Review Request 127215: simplify code, reduce pointer dereferences

[Nick Shaforostoff]

> On Feb. 29, 2016, 8:14 a.m., David Faure wrote:
> > src/sycoca/kbuildmimetypefactory.cpp, line 65
> > <https://git.reviewboard.kde.org/r/127215/diff/1/?file=445933#file445933line65>
> >
> > Can you keep the order of the operands? I find
> >   if (var == constant)
> > more readable than the other way around.
> > ("is this car green?", not "is green the color of this car?"). In any 
> > case that's how it is everywhere in KF5 AFAIK.
> 
> Nick Shaforostoff wrote:
> i have changed to a natural order. but the problem is that i couldn't 
> find an appropriate operator== overload taking (qstringref, qlatin1string), 
> there is only a (qlatin1string, qstringref) one. is that a qt bug that should 
> be fixed?
> 
> David Faure wrote:
> Sounds like it.

i have reported the bug, but it was closed as invalid: turns out there is such 
overload, but it is not reflected in the reference


- Nick


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


On Feb. 29, 2016, 12:27 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127215/
> ---
> 
> (Updated Feb. 29, 2016, 12:27 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Changes are mostly related to containers/iterators, but there are also few 
> QString related optimizations
> 
> note that i have made a switch from QLinkedList to just QList because it 
> performs better for T=
> also i have changed QStringList to QSet in one place to make the 
> code run faster
> 
> 
> Diffs
> -
> 
>   src/services/kservice.cpp c75bce2 
>   src/services/kservicetype.cpp 2a73ccd 
>   src/sycoca/kbuildmimetypefactory.cpp 340f76a 
>   src/sycoca/kbuildservicefactory.cpp a74f2e8 
>   src/sycoca/kbuildsycoca.cpp e99e906 
>   src/sycoca/kmimeassociations.cpp 9423b27 
>   src/sycoca/ksycocadict.cpp f33d01a 
>   src/sycoca/ksycocafactory.cpp e410581 
>   src/sycoca/ksycocautils_p.h 8db26b0 
> 
> Diff: https://git.reviewboard.kde.org/r/127215/diff/
> 
> 
> Testing
> ---
> 
> all tests pass, except kmimeassociationstest (fakeApplicationService is 
> NULL), but it fails also without my changes
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

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

Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[Nick Shaforostoff]

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


Ship it!




great finding, thanks for you work!

- Nick Shaforostoff


On March 1, 2016, 3:25 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127236/
> ---
> 
> (Updated March 1, 2016, 3:25 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> At the moment we're playing Battleship to see if an icon is present in a 
> subdirectory. This means that we are checking on every directory if there's 
> an icon that matches the size with a said name on every request.
> 
> This can be seen easily with strace:
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
> ```
> After the patch: 
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
> ```
> We reduce these accesses to let QDir keep the list of files inside the 
> directory (that was already being generated at some point, it just was being 
> discarded).
> 
> 
> Diffs
> -
> 
>   src/kicontheme.cpp 0996054 
> 
> Diff: https://git.reviewboard.kde.org/r/127236/diff/
> 
> 
> Testing
> ---
> 
> Builds, tests still pass, applications start noticeably faster.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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

Re: Review Request 127215: simplify code, reduce pointer dereferences

[Nick Shaforostoff]

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



so, any objections for submitting the patch?

- Nick Shaforostoff


On Feb. 29, 2016, 12:27 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127215/
> ---
> 
> (Updated Feb. 29, 2016, 12:27 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Changes are mostly related to containers/iterators, but there are also few 
> QString related optimizations
> 
> note that i have made a switch from QLinkedList to just QList because it 
> performs better for T=
> also i have changed QStringList to QSet in one place to make the 
> code run faster
> 
> 
> Diffs
> -
> 
>   src/services/kservice.cpp c75bce2 
>   src/services/kservicetype.cpp 2a73ccd 
>   src/sycoca/kbuildmimetypefactory.cpp 340f76a 
>   src/sycoca/kbuildservicefactory.cpp a74f2e8 
>   src/sycoca/kbuildsycoca.cpp e99e906 
>   src/sycoca/kmimeassociations.cpp 9423b27 
>   src/sycoca/ksycocadict.cpp f33d01a 
>   src/sycoca/ksycocafactory.cpp e410581 
>   src/sycoca/ksycocautils_p.h 8db26b0 
> 
> Diff: https://git.reviewboard.kde.org/r/127215/diff/
> 
> 
> Testing
> ---
> 
> all tests pass, except kmimeassociationstest (fakeApplicationService is 
> NULL), but it fails also without my changes
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

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

Re: Review Request 127215: simplify code, reduce pointer dereferences

[Nick Shaforostoff]

> On Feb. 29, 2016, 12:31 p.m., Kai Uwe Broulik wrote:
> > src/sycoca/kbuildservicefactory.cpp, line 167
> > <https://git.reviewboard.kde.org/r/127215/diff/2/?file=445996#file445996line167>
> >
> > We can't use range-for in Frameworks, right?

in this case i benefit from Q_FOREACH taking a copy of the container


> On Feb. 29, 2016, 12:31 p.m., Kai Uwe Broulik wrote:
> > src/sycoca/kbuildsycoca.cpp, line 143
> > <https://git.reviewboard.kde.org/r/127215/diff/2/?file=445997#file445997line143>
> >
> > QVector maybe?

doesn't make a difference from performance perspective, see 
https://marcmutz.wordpress.com/effective-qt/containers/
but just changing qlist to qvector here increases the lib binary size by 15kb 
(!)


> On Feb. 29, 2016, 12:31 p.m., Kai Uwe Broulik wrote:
> > src/sycoca/kbuildsycoca.cpp, lines 246-248
> > <https://git.reviewboard.kde.org/r/127215/diff/2/?file=445997#file445997line246>
> >
> > Cache end?

that's too much ))
in other places caching did make sence because container was accessed via 
pointer


- Nick


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


On Feb. 29, 2016, 12:27 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127215/
> ---
> 
> (Updated Feb. 29, 2016, 12:27 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Changes are mostly related to containers/iterators, but there are also few 
> QString related optimizations
> 
> note that i have made a switch from QLinkedList to just QList because it 
> performs better for T=
> also i have changed QStringList to QSet in one place to make the 
> code run faster
> 
> 
> Diffs
> -
> 
>   src/services/kservice.cpp c75bce2 
>   src/services/kservicetype.cpp 2a73ccd 
>   src/sycoca/kbuildmimetypefactory.cpp 340f76a 
>   src/sycoca/kbuildservicefactory.cpp a74f2e8 
>   src/sycoca/kbuildsycoca.cpp e99e906 
>   src/sycoca/kmimeassociations.cpp 9423b27 
>   src/sycoca/ksycocadict.cpp f33d01a 
>   src/sycoca/ksycocafactory.cpp e410581 
>   src/sycoca/ksycocautils_p.h 8db26b0 
> 
> Diff: https://git.reviewboard.kde.org/r/127215/diff/
> 
> 
> Testing
> ---
> 
> all tests pass, except kmimeassociationstest (fakeApplicationService is 
> NULL), but it fails also without my changes
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

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

Re: Review Request 127215: simplify code, reduce pointer dereferences

[Nick Shaforostoff]

> On Feb. 29, 2016, 4:12 a.m., Aleix Pol Gonzalez wrote:
> > src/sycoca/kbuildsycoca.cpp, line 246
> > <https://git.reviewboard.kde.org/r/127215/diff/1/?file=445935#file445935line246>
> >
> > foreach like you did above?

for loops with simple body i prefer direct iterators to a copy-taking Q_FOREACH


- Nick


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


On Feb. 29, 2016, 12:27 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127215/
> ---
> 
> (Updated Feb. 29, 2016, 12:27 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Changes are mostly related to containers/iterators, but there are also few 
> QString related optimizations
> 
> note that i have made a switch from QLinkedList to just QList because it 
> performs better for T=
> also i have changed QStringList to QSet in one place to make the 
> code run faster
> 
> 
> Diffs
> -
> 
>   src/services/kservice.cpp c75bce2 
>   src/services/kservicetype.cpp 2a73ccd 
>   src/sycoca/kbuildmimetypefactory.cpp 340f76a 
>   src/sycoca/kbuildservicefactory.cpp a74f2e8 
>   src/sycoca/kbuildsycoca.cpp e99e906 
>   src/sycoca/kmimeassociations.cpp 9423b27 
>   src/sycoca/ksycocadict.cpp f33d01a 
>   src/sycoca/ksycocafactory.cpp e410581 
>   src/sycoca/ksycocautils_p.h 8db26b0 
> 
> Diff: https://git.reviewboard.kde.org/r/127215/diff/
> 
> 
> Testing
> ---
> 
> all tests pass, except kmimeassociationstest (fakeApplicationService is 
> NULL), but it fails also without my changes
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

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

Re: Review Request 127215: simplify code, reduce pointer dereferences

[Nick Shaforostoff]

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

(Updated Feb. 29, 2016, 12:27 p.m.)


Review request for KDE Frameworks.


Changes
---

followed the suggestions, and also changed contains(QStringLiteral(...)) to 
contains(QLatin1String(...))


Repository: kservice


Description
---

Changes are mostly related to containers/iterators, but there are also few 
QString related optimizations

note that i have made a switch from QLinkedList to just QList because it 
performs better for T=
also i have changed QStringList to QSet in one place to make the code 
run faster


Diffs (updated)
-

  src/services/kservice.cpp c75bce2 
  src/services/kservicetype.cpp 2a73ccd 
  src/sycoca/kbuildmimetypefactory.cpp 340f76a 
  src/sycoca/kbuildservicefactory.cpp a74f2e8 
  src/sycoca/kbuildsycoca.cpp e99e906 
  src/sycoca/kmimeassociations.cpp 9423b27 
  src/sycoca/ksycocadict.cpp f33d01a 
  src/sycoca/ksycocafactory.cpp e410581 
  src/sycoca/ksycocautils_p.h 8db26b0 

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


Testing
---

all tests pass, except kmimeassociationstest (fakeApplicationService is NULL), 
but it fails also without my changes


Thanks,

Nick Shaforostoff

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

Review Request 127215: simplify code, reduce pointer dereferences

[Nick Shaforostoff]

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

Review request for KDE Frameworks.


Repository: kservice


Description
---

Changes are mostly related to containers/iterators, but there are also few 
QString related optimizations

note that i have made a switch from QLinkedList to just QList because it 
performs better for T=
also i have changed QStringList to QSet in one place to make the code 
run faster


Diffs
-

  src/sycoca/kbuildmimetypefactory.cpp 340f76a 
  src/sycoca/kbuildservicefactory.cpp a74f2e8 
  src/sycoca/kbuildsycoca.cpp e99e906 
  src/sycoca/kmimeassociations.cpp 9423b27 
  src/sycoca/ksycocadict.cpp f33d01a 
  src/sycoca/ksycocafactory.cpp e410581 

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


Testing
---

all tests pass, except kmimeassociationstest (fakeApplicationService is NULL), 
but it fails also without my changes


Thanks,

Nick Shaforostoff

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

Re: QString -> QStringLiteral conversions might make applications crash on exit

[Nick Shaforostoff]

> a) Everyone who makes QString -> QStringLiteral replacements should be
> extremely careful (which is very difficult, since it is not always
> obvious if passing a QString to a function will result in the string
> being stored in a global static object). Automated tools like clazy
> should then not recommend to use QStringLiteral any more.
> 
> b) Classes like KIconLoader, which are used as global static objects,
> should copy all strings that they get to the heap in order to prevent
> such crashes (which might also be quite difficult to do consistently).

from what i have seen, at least icons, fonts and regexps are the ones for which 
stringliterals should be avoided in libraries.
i have created a qt issue for mentioning this in qt documentation
https://bugreports.qt.io/browse/QTBUG-51418 

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

Re: Review Request 126774: Fix many warnings presented by clazy

[Nick Shaforostoff]

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




src/core/kprotocolinfofactory.cpp (line 114)
<https://git.reviewboard.kde.org/r/126774/#comment63125>

please use QLatin1String here: there is a special overload for it



src/core/kprotocolmanager.cpp (line 569)
<https://git.reviewboard.kde.org/r/126774/#comment63126>

please revert back to QLatin1String here, s there is special overload for it



src/core/kprotocolmanager.cpp (line 838)
<https://git.reviewboard.kde.org/r/126774/#comment63127>

please revert back to QLatin1String, as there is a special overload for it. 
if you really want to reduce number of [re]allocs then change it to one big 
assignment:

d->useragent = ... + ... + ... + ...;

and make sure QT_USE_STRINGBUILDER is defined



src/core/kprotocolmanager.cpp (line 940)
<https://git.reviewboard.kde.org/r/126774/#comment63128>

please revert back to QLatin1String, as there is a special overload for it. 
if you really want to reduce number of [re]allocs then change it to one big 
assignment:

info += ... + ... + ... + ...;

and make sure QT_USE_STRINGBUILDER is defined


- Nick Shaforostoff


On Feb. 20, 2016, 8:41 p.m., Russell Greene wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126774/
> ---
> 
> (Updated Feb. 20, 2016, 8:41 p.m.)
> 
> 
> Review request for KDE Frameworks and Aleix Pol Gonzalez.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Fix many warnings presented by clazy
> 
> I ran the clazy static anaylizer on the codebase and fixed many of the errors.
> 
> Many of these were using QStringLiteral instead of QString, which removes an 
> allocation, including Q_OBJECT macros, adding const ref instad of const 
> lvalue, stuff like that.
> 
> 
> Diffs
> -
> 
>   autotests/http/httpheaderdispositiontest.cpp ac41656 
>   autotests/http/httpheadertokenizetest.cpp e57be09 
>   autotests/jobtest.cpp dfbfba9 
>   autotests/kacltest.cpp 2b88906 
>   autotests/kcookiejar/kcookiejartest.cpp dab6987 
>   autotests/kfileitemtest.cpp 49d11e1 
>   autotests/kfileplacesmodeltest.cpp effc95b 
>   autotests/klocalsocketservertest.cpp 9b7f246 
>   autotests/klocalsockettest.cpp 0d831f2 
>   autotests/kmountpointtest.cpp f6eccd8 
>   autotests/knewfilemenutest.cpp 1b11f15 
>   autotests/kprotocolinfotest.cpp 812f7f7 
>   autotests/ktcpsockettest.cpp 640d871 
>   autotests/kurlnavigatortest.cpp 1b295c3 
>   autotests/udsentry_benchmark.cpp 16c8b7e 
>   src/core/dataprotocol_p.h 784226b 
>   src/core/global.cpp eaa4264 
>   src/core/job_error.cpp 7943cb3 
>   src/core/kprotocolinfofactory.cpp 1329b6b 
>   src/core/kprotocolmanager.cpp bd42c9e 
>   src/core/ksambasharedata.cpp b63f518 
>   src/core/ksslcertificatemanager.cpp 07feda4 
>   src/core/restorejob.cpp ee5cca9 
>   src/core/sessiondata.cpp d21ab21 
>   src/core/simplejob.cpp 440fa62 
>   src/core/slave.cpp 5ae4d97 
>   src/core/slavebase.cpp a9bf648 
>   src/core/statjob.cpp e55e3b4 
>   src/core/storedtransferjob.cpp 3e86cb9 
>   src/core/tcpslavebase.cpp 2bd1c0f 
>   src/filewidgets/defaults-kfile.h 825a297 
>   src/filewidgets/defaultviewadapter_p.h 79381e4 
>   src/filewidgets/kdiroperator.cpp 9c2b6be 
>   src/filewidgets/kfilecopytomenu.cpp dab5db6 
>   src/filewidgets/kfileplacesitem.cpp 1748188 
>   src/filewidgets/kfileplacesmodel.cpp e9d37a9 
>   src/filewidgets/kfileplacesview.cpp 4121191 
>   src/filewidgets/knewfilemenu.cpp b249898 
>   src/filewidgets/kurlnavigator.cpp 64d2a6d 
>   src/ioslaves/ftp/ftp.cpp 7477a6a 
>   src/ioslaves/http/http.cpp 76da711 
>   src/ioslaves/http/kcookiejar/kcookiejar.cpp c06882a 
>   src/ioslaves/http/kcookiejar/kcookieserver.cpp 3e7dd8c 
>   src/ioslaves/trash/kcmtrash.cpp 79c2ca7 
>   src/ioslaves/trash/ktrash.cpp 298a201 
>   src/ioslaves/trash/tests/testtrash.cpp 339aa19 
>   src/ioslaves/trash/trashimpl.cpp 26d9ea8 
>   src/kcms/kio/kcookiespolicyselectiondlg.cpp fcd0763 
>   src/kcms/kio/kproxydlg.cpp fe2583e 
>   src/kcms/kio/main.cpp 387cf89 
>   src/kcms/kio/useragentdlg.cpp a6560b5 
>   src/kcms/kio/useragentinfo.cpp 1459fcc 
>   src/kcms/kio/useragentselectordlg.cpp 3952761 
>   src/kioexec/main.cpp 6f4540f 
>   src/kntlm/kntlm.cpp ed6f388 
>   src/kpac/script.cpp 9b126ca 
>   src/kpasswdserver/kpasswdserver.cpp 7a53300 
>   src/kssld/kssld.cpp fcc3bed 
>   src/protocoltojson/main

Re: Review Request 126392: Fix some Clazy warnings in kcoreaddons

[Nick Shaforostoff]

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



src/lib/io/kbackup.cpp (line 86)
<https://git.reviewboard.kde.org/r/126392/#comment61707>

because a string is appended to the existing one, i suggest to use 
QLatin1String here (there is an overload taking QLatin1String as an argument)



src/lib/io/kprocess.cpp (line 241)
<https://git.reviewboard.kde.org/r/126392/#comment61708>

because a string is appended to the existing one, i suggest to use 
QLatin1String here (there is an overload taking QLatin1String as an argument)


- Nick Shaforostoff


On Dec. 24, 2015, 11:58 a.m., Andrey Cygankov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126392/
> ---
> 
> (Updated Dec. 24, 2015, 11:58 a.m.)
> 
> 
> Review request for KDE Frameworks and Aleix Pol Gonzalez.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Fix some Clazy warnings:
> - unneeded heap allocation with QString
> - midRef() non-usage
> - isEmpty() non-usage
> 
> 
> Diffs
> -
> 
>   src/lib/io/kbackup.h ead2de5 
>   autotests/ktexttohtmltest.cpp 81c3fc0 
>   src/lib/io/kprocess.cpp 1df61c0 
>   src/lib/io/kmessage.cpp 305318c 
>   src/lib/io/kdirwatch.cpp 34b32b7 
>   src/lib/io/kdirwatch_p.h 4483961 
>   src/lib/io/kbackup.cpp ec8b8b2 
>   src/lib/io/kurlmimedata.cpp 4095ee1 
>   src/lib/kaboutdata.h 0dbd7a0 
>   src/lib/kaboutdata.cpp 5d9a55e 
>   src/lib/randomness/krandom.cpp 2eb80d2 
>   src/lib/text/kmacroexpander.cpp 0d05ffd 
>   src/lib/text/kstringhandler.cpp 826ddcb 
>   src/lib/text/ktexttohtml.cpp d58f5c0 
>   src/lib/util/kshell.cpp 11892ce 
> 
> Diff: https://git.reviewboard.kde.org/r/126392/diff/
> 
> 
> Testing
> ---
> 
> Compiling without errors
> 
> 
> Thanks,
> 
> Andrey Cygankov
> 
>

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

Re: Review Request 126348: Make it possible to provide the metadata in json

[Nick Shaforostoff]

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



src/kpackage/private/packages.cpp (line 57)
<https://git.reviewboard.kde.org/r/126348/#comment61260>

something more meaningful?


- Nick Shaforostoff


On Dec. 14, 2015, 5:11 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126348/
> ---
> 
> (Updated Dec. 14, 2015, 5:11 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: kpackage
> 
> 
> Description
> ---
> 
> Due to KCoreAddons transition from using desktop files to json files, 
> KPackage ended in a weird state where desktop files were allowed to use 
> desktop files but not json.
> 
> This patch makes sure that manifest.json files are also acceptable.
> 
> 
> Diffs
> -
> 
>   autotests/querytest.cpp 0a2f11a 
>   src/kpackage/package.cpp 8416054 
>   src/kpackage/packageloader.cpp 1e1e382 
>   src/kpackage/private/packagejobthread.cpp 444dacc 
>   src/kpackage/private/packagejobthread_p.h 1babf0b 
>   src/kpackage/private/packages.cpp 2f6bbcf 
> 
> Diff: https://git.reviewboard.kde.org/r/126348/diff/
> 
> 
> Testing
> ---
> 
> Tests still pass (except for 
> https://build.kde.org/job/kpackage%20master%20stable-kf5-qt5/37/PLATFORM=Linux,compiler=gcc/testReport/,
>  that is not passing in master).
> Adds a test plugin with a json file in it.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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

Re: Review Request 126350: Fixed all Clazy level 1 and level 2 warnings

[Nick Shaforostoff]

> On Dec. 14, 2015, 7:10 p.m., Sergio Martins wrote:
> > src/klocalizedstring.cpp, line 389
> > 
> >
> > this will break the build with MSVC, it doesn't support QStringLiteral 
> > with multi-line literals.
> > 
> > (hint:
> > export 
> > CLAZY_EXTRA_OPTIONS="qstring-uneeded-heap-allocations-msvc-compat"
> > )

why QString is used for debug output in the first place?
there is operator<<(const char * s) for latin1 text, which is the case
(i.e. no non-latin symbols are used in the literals)

using strings for debugging output is suboptimal


- Nick


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


On Dec. 14, 2015, 6:46 p.m., Artur Puzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126350/
> ---
> 
> (Updated Dec. 14, 2015, 6:46 p.m.)
> 
> 
> Review request for KDE Frameworks and Aleix Pol Gonzalez.
> 
> 
> Repository: ki18n
> 
> 
> Description
> ---
> 
> Fixed `warning: KuitSetup has dtor, copy-ctor but not copy-assignment 
> [-Wclazy-rule-of-three]`
> Fixed `warning: Reserve candidate [-Wclazy-reserve-candidates]`
> Fixed `warning: Use QVariant::toFoo() instead of QVariant::value() 
> [-Wclazy-variant-sanitizer]`
> Fixed `warning: Use midRef() instead [-Wclazy-qstring-ref]`
> Fixed `warning: Pass small and trivially-copyable type by value (const class 
> QChar &) [-Wclazy-foreach]`
> Fixed `warning: QString::fromLatin1() being passed a literal 
> [-Wclazy-qstring-uneeded-heap-allocations]`
> Fixed `warning: QString(const char*) being called 
> [-Wclazy-qstring-uneeded-heap-allocations]`
> Fixed `warning: QString(QLatin1String) being called 
> [-Wclazy-qstring-uneeded-heap-allocations]`
> 
> 
> Diffs
> -
> 
>   src/kcatalog.cpp 7711e9b 
>   src/klocalizedcontext.cpp 3bc42dd 
>   src/common_helpers.cpp dad7f84 
>   src/klocalizedstring.cpp 69950d2 
>   src/ktranscript.cpp 04dda66 
>   src/kuitmarkup.h d110ca3 
>   src/kuitmarkup.cpp 02b891a 
> 
> Diff: https://git.reviewboard.kde.org/r/126350/diff/
> 
> 
> Testing
> ---
> 
> Automated tests 1,3 and 4 passing.
> Test 2 fails on my system both: after changes and before.
> 
> 
> Thanks,
> 
> Artur Puzio
> 
>

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

Re: Review Request 126328: kwallet: Do not use QStringLiteral with multi strings

[Nick Shaforostoff]

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


i suggest using:

#ifdef Q_OS_WIN
#define U QLatin1String
#else
#define U QStringLiteral
#endif

- Nick Shaforostoff


On Dec. 12, 2015, 10:59 p.m., Patrick Spendrin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126328/
> ---
> 
> (Updated Dec. 12, 2015, 10:59 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kwallet
> 
> 
> Description
> ---
> 
> Strings that are separated into multiple parts don't work on Windows
> together with QStringLiteral as the first string is interpreted as a
> wide (16bit) string, and the second one as a narrow (8bit) string.
> Replacing with QString::fromLatin1 is the easiest solution keeping
> the code layout the same, joining the strings does work too though.
> 
> 
> Diffs
> -
> 
>   src/runtime/kwalletd/kwalletd.cpp 5f99f161a0911732c4d46ab36f2e4f3d3f3e3c4b 
> 
> Diff: https://git.reviewboard.kde.org/r/126328/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Patrick Spendrin
> 
>

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

Re: Review Request 126327: sonnet: Do not use QStringLiteral with multi strings

[Nick Shaforostoff]

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

Ship it!


as that are just tests, i.e. not real code, i suggest just using QLatin1String 
everywhere

- Nick Shaforostoff


On Dec. 12, 2015, 10:54 p.m., Patrick Spendrin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126327/
> ---
> 
> (Updated Dec. 12, 2015, 10:54 p.m.)
> 
> 
> Review request for KDE Frameworks and Martin Tobias Holmedahl Sandsmark.
> 
> 
> Repository: sonnet
> 
> 
> Description
> ---
> 
> Strings that are separated into multiple parts don't work on Windows
> together with QStringLiteral as the first string is interpreted as a
> wide (16bit) string, and the second one as a narrow (8bit) string.
> Replacing with QString::fromLatin1 is the easiest solution keeping
> the code layout the same, joining the strings does work too though.
> 
> 
> Diffs
> -
> 
>   examples/dialogexample.cpp 4bda1ad4c2e8df1d51e7007838fbc43747919304 
>   examples/plaintextedit.cpp 45b5503612cbdb32796228bcee1b53a472e9d313 
>   examples/textedit.cpp a1ccb28aab35e2491b1512241a05767429ce84d9 
> 
> Diff: https://git.reviewboard.kde.org/r/126327/diff/
> 
> 
> Testing
> ---
> 
> Windows.
> 
> 
> Thanks,
> 
> Patrick Spendrin
> 
>

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

Re: Review Request 126329: kio: Do not use QStringLiteral with multi strings

[Nick Shaforostoff]

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


please use QLatin1String instead of QString::fromLatin1 for QRegExp.
for the rest i suggest using

#ifdef Q_OS_WIN
#define U QLatin1String
#else
#define U QStringLiteral
#endif

or smth like that.

- Nick Shaforostoff


On Dec. 12, 2015, 11:08 p.m., Patrick Spendrin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126329/
> ---
> 
> (Updated Dec. 12, 2015, 11:08 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Strings that are separated into multiple parts don't work on Windows
> together with QStringLiteral as the first string is interpreted as a
> wide (16bit) string, and the second one as a narrow (8bit) string.
> Replacing with QString::fromLatin1 is the easiest solution keeping
> the code layout the same, joining the strings does work too though.
> 
> 
> Diffs
> -
> 
>   autotests/dataprotocoltest.cpp 9fe238fdbb0e9682141772d423a64edd5621921b 
>   src/core/ksambashare.cpp a3f84ac3971141e687d9ab17e0131a66db34ed5a 
>   src/filewidgets/kfileplacesmodel.cpp 
> b409c1b1617f97f3cdbc79a2c76110a5f9449398 
>   src/ioslaves/help/kio_help.cpp cb27a77b22fe378a126d985621985265edb93767 
>   src/widgets/kpropertiesdialog.cpp 0ff506273a10dba238fefc5c552c71434681285e 
> 
> Diff: https://git.reviewboard.kde.org/r/126329/diff/
> 
> 
> Testing
> ---
> 
> Windows.
> 
> 
> Thanks,
> 
> Patrick Spendrin
> 
>

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

Re: Review Request 126140: [kfilemetadata] avoid utf16->utf8->utf16 conversions reading taglib metadata

[Nick Shaforostoff]

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

(Updated Nov. 24, 2015, 11:05 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, Juan Palacios, Pino Toscano, and Vishesh 
Handa.


Changes
---

Submitted with commit 4655943a49acfa181d31a00c5aef1bf10dcba2bf by Nick 
Shaforostoff to branch master.


Repository: kfilemetadata


Description
---

also this patch marks 'audio/mp4' mimetype as supported by taglib for reading 
.m4a files (AAC or ALAC)


Diffs
-

  src/extractors/taglibextractor.cpp 3bf33cd 

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


Testing
---

tested with Spanish accents (e.g. José, Cátulo) in the tags.

also i did test this toCWString-based conversion on windows and it worked fine. 
should work on osx as well


Thanks,

Nick Shaforostoff

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

Re: Review Request 126140: [kfilemetadata] avoid utf16->utf8->utf16 conversions reading taglib metadata

[Nick Shaforostoff]

> On Nov. 24, 2015, 10:12 p.m., David Faure wrote:
> > Looks good, but should be two different commits of course.
> > 
> > Would this have been caught by -DQT_NO_CAST_FROM_ASCII?

no, i used this conversion in my pet project for organizing tango music 
collection and just wondered if kfilemetadata does the same.

two different commits - ok.


- Nick


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


On Nov. 23, 2015, 9:01 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126140/
> ---
> 
> (Updated Nov. 23, 2015, 9:01 p.m.)
> 
> 
> Review request for KDE Frameworks, Juan Palacios, Pino Toscano, and Vishesh 
> Handa.
> 
> 
> Repository: kfilemetadata
> 
> 
> Description
> ---
> 
> also this patch marks 'audio/mp4' mimetype as supported by taglib for reading 
> .m4a files (AAC or ALAC)
> 
> 
> Diffs
> -
> 
>   src/extractors/taglibextractor.cpp 3bf33cd 
> 
> Diff: https://git.reviewboard.kde.org/r/126140/diff/
> 
> 
> Testing
> ---
> 
> tested with Spanish accents (e.g. José, Cátulo) in the tags.
> 
> also i did test this toCWString-based conversion on windows and it worked 
> fine. should work on osx as well
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

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

Review Request 126140: [kfilemetadata] avoid utf16->utf8->utf16 conversions reading taglib metadata

[Nick Shaforostoff]

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

Review request for KDE Frameworks and Vishesh Handa.


Repository: kfilemetadata


Description
---

also this patch marks 'audio/mp4' mimetype as supported by taglib for reading 
.m4a files (AAC or ALAC)


Diffs
-

  src/extractors/taglibextractor.cpp 3bf33cd 

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


Testing
---

tested with Spanish accents (e.g. José, Cátulo) in the tags.

also i did test this toCWString-based conversion on windows and it worked fine. 
should work on osx as well


Thanks,

Nick Shaforostoff

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

Re: Review Request 125951: [calendar] Move the plugins handling to a separate class

[Nick Shaforostoff]

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



src/declarativeimports/calendar/eventpluginsmanager.cpp (line 42)
<https://git.reviewboard.kde.org/r/125951/#comment60491>

please use QByteArrayLiteral("configUi")



src/declarativeimports/calendar/eventpluginsmanager.cpp (line 68)
<https://git.reviewboard.kde.org/r/125951/#comment60490>

QStringLiteral("MetaData"). this is important because data() method is 
called a lot.



src/declarativeimports/calendar/eventpluginsmanager.cpp (line 79)
<https://git.reviewboard.kde.org/r/125951/#comment60489>

in order to reduce malloc/free call count (and the data() method is called 
a lot) please use leftRef() instead of left() and % instead of + three lines 
below (or make sure + is the same as % by adding define -DQT_USE_QSTRINGBUILDER)


- Nick Shaforostoff


On Nov. 9, 2015, 7:56 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125951/
> ---
> 
> (Updated Nov. 9, 2015, 7:56 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> This is also made a QML singleton that will be used for the applet
> config view where it will add the plugin configs once we add that
> possibility.
> 
> The same instance is then set to the DaysModel from QML.
> 
> (this depends on https://git.reviewboard.kde.org/r/125817/ which awaits ship 
> it)
> 
> 
> Diffs
> -
> 
>   src/declarativeimports/calendar/CMakeLists.txt 
> 40ead911ad5208cae5dbe5333d227f9f8a0d9154 
>   src/declarativeimports/calendar/calendarplugin.cpp 
> bafe80cf7520a08312abfd1dbd6d4648a6710175 
>   src/declarativeimports/calendar/daysmodel.h 
> a5bdac98627f7efa76bd4afd239469b53e06690b 
>   src/declarativeimports/calendar/daysmodel.cpp 
> 2d059a8e8636565adbe52811e602fff37a5eb157 
>   src/declarativeimports/calendar/eventpluginsmanager.h PRE-CREATION 
>   src/declarativeimports/calendar/eventpluginsmanager.cpp PRE-CREATION 
>   src/declarativeimports/calendar/qml/MonthView.qml 
> f698934f850ef3a917b9611c9f9a40c369b23f6c 
> 
> Diff: https://git.reviewboard.kde.org/r/125951/diff/
> 
> 
> Testing
> ---
> 
> Calendar events are still correctly displayed
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

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

Re: Review Request 126002: Fix KArchive for behavior change in Qt 5.6

[Nick Shaforostoff]

> On Nov. 9, 2015, 10:42 a.m., Nick Shaforostoff wrote:
> > src/ktar.cpp, line 293
> > <https://git.reviewboard.kde.org/r/126002/diff/1/?file=415780#file415780line293>
> >
> > why can't we just use QFile::decodeName(const char * localFileName) 
> > overload?

ok, there is a limit of 100. then i suggest using 
QByteArray::fromRawData(buffer, qstrnlen(buffer, 100)) because qbytearray 
object is 100% temporary.


- Nick


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


On Nov. 9, 2015, 7:54 a.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126002/
> ---
> 
> (Updated Nov. 9, 2015, 7:54 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: karchive
> 
> 
> Description
> ---
> 
> Fix KArchive for behavior change in Qt 5.6
> 
> Embedded NULs are now preserved when converting from QByteArray to QString.
> 
> + remove unused var (separate commit)
> 
> 
> Diffs
> -
> 
>   autotests/karchivetest.cpp d9662f5faadf9f08f71a386c0f9ecc8ef89af23a 
>   src/karchive.cpp e0a1eb2245767f9d605c44a6da481d290c2b15a5 
>   src/ktar.cpp 824395e72e73b23f974614d5eff1fa58c7729dc0 
> 
> Diff: https://git.reviewboard.kde.org/r/126002/diff/
> 
> 
> Testing
> ---
> 
> karchivetest passes again, with Qt 5.6 from git.
> 
> I complained about the behavior change on 
> https://codereview.qt-project.org/106473
> 
> 
> Thanks,
> 
> David Faure
> 
>

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

Re: Review Request 126002: Fix KArchive for behavior change in Qt 5.6

[Nick Shaforostoff]

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



src/ktar.cpp (line 292)
<https://git.reviewboard.kde.org/r/126002/#comment60472>

why can't we just use QFile::decodeName(const char * localFileName) 
overload?


- Nick Shaforostoff


On Nov. 9, 2015, 7:54 a.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126002/
> ---
> 
> (Updated Nov. 9, 2015, 7:54 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: karchive
> 
> 
> Description
> ---
> 
> Fix KArchive for behavior change in Qt 5.6
> 
> Embedded NULs are now preserved when converting from QByteArray to QString.
> 
> + remove unused var (separate commit)
> 
> 
> Diffs
> -
> 
>   autotests/karchivetest.cpp d9662f5faadf9f08f71a386c0f9ecc8ef89af23a 
>   src/karchive.cpp e0a1eb2245767f9d605c44a6da481d290c2b15a5 
>   src/ktar.cpp 824395e72e73b23f974614d5eff1fa58c7729dc0 
> 
> Diff: https://git.reviewboard.kde.org/r/126002/diff/
> 
> 
> Testing
> ---
> 
> karchivetest passes again, with Qt 5.6 from git.
> 
> I complained about the behavior change on 
> https://codereview.qt-project.org/106473
> 
> 
> Thanks,
> 
> David Faure
> 
>

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

Re[2]: Failure while executing KTar::open while using KCompressionDevice as the device

[Nick Shaforostoff]

 > Aleix suggested QBuffer, but archives can be *huge*, so this might
> eat all available memory. This is the reason why KTar uses a temp file
> for the compressed-tar case.

qbuffer may be used for decompressing archives that are e.g. less than 50% of 
free ram
if an archive is larger then tmpfile-based branch would be executed.
 
 
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125971: k7zip: fix memleaks, lower memory usage

[Nick Shaforostoff]

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

(Updated Nov. 7, 2015, 10:07 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, David Faure and Laurent Montel.


Changes
---

Submitted with commit 5e00ad1f31a6cb436f3d17896de82e1691acfefa by Nick 
Shaforostoff to branch master.


Repository: karchive


Description
---

i couldn't find the place where the pointers contained in the member arrays are 
deleted so i have added their releasing. for this i have used qDeleteAll (you 
can search for qDeleteAll in the diff)

also i have reordered members of FileInfo to reduce its 'sizeof'
also using qvector for storing pointers is as fast as using qlist (or even 
faster) and needs less memory
also i have switched qvector acces from operator[] to .at() because it is const 
(doesn't call detach() method internally)
also i have disabled the code that was filling 'method' string because it was 
not used anywhere after


Diffs
-

  src/k7zip.cpp 321620a 
  src/karchive.cpp 0ece37c 

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


Testing
---

compiles fine


Thanks,

Nick Shaforostoff

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

Re: Review Request 125969: kinit: fix Coverity issues + small optimization

[Nick Shaforostoff]

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

(Updated Nov. 7, 2015, 12:04 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Alex Merry.


Changes
---

Submitted with commit 04e64f8ec47476530157cefc434776f4aa93a27f by Nick 
Shaforostoff to branch master.


Repository: kinit


Description
---

this patch fixes two coverity issues ranked 'outstanding':
https://scan5.coverity.com/reports.htm#v39099/p10103/fileInstanceId=82663767&defectInstanceId=24554334&mergedDefectId=258481
https://scan5.coverity.com/reports.htm#v39099/p10103/fileInstanceId=82663767&defectInstanceId=24557588&mergedDefectId=258474

and also does small string-related optimization by eliminating redundant 
mallocs done by QByteArray ctor


Diffs
-

  src/kdeinit/kinit.cpp 9e775b6 

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


Testing
---

compiles fine


Thanks,

Nick Shaforostoff

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

Re: Review Request 125815: Provide an alternative when khelpcenter is not available

[Nick Shaforostoff]

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



src/util/urlhandler.cpp (line 49)
<https://git.reviewboard.kde.org/r/125815/#comment60442>

there is no point in using QStringLiteral there as a deep copy is done 
anyways. using QLatin1String would take almost the same time, except that the 
.so binary would be smaller.

instead for QStandardPaths::findExecutable it does make sense to use 
QStringLiteral


- Nick Shaforostoff


On Nov. 6, 2015, 4:48 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125815/
> ---
> 
> (Updated Nov. 6, 2015, 4:48 p.m.)
> 
> 
> Review request for KDE Frameworks, Christoph Cullmann and Luigi Toscano.
> 
> 
> Repository: kguiaddons
> 
> 
> Description
> ---
> 
> Tries to reach out to docs.kde.org when khelpcenter is not available.
> 
> 
> Diffs
> -
> 
>   src/util/urlhandler.cpp 5b46be2 
> 
> Diff: https://git.reviewboard.kde.org/r/125815/diff/
> 
> 
> Testing
> ---
> 
> I tried it with a couple of applications. For the main documentation just 
> works.
> 
> Needs figuring out for more complex cases, I'm unsure if applications are 
> opening the documentation in specific pages. In fact, I couldn't find the 
> documentation for docs.kde.org url scheme, and I just made up the `path` 
> part, although it seems to work.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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

Re: Review Request 125976: add an update() method

[Nick Shaforostoff]

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



src/kpackage/private/packagejobthread.cpp (line 320)
<https://git.reviewboard.kde.org/r/125976/#comment60441>

QLatin1String("/metadata.desktop")


- Nick Shaforostoff


On Nov. 6, 2015, 4:10 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125976/
> ---
> 
> (Updated Nov. 6, 2015, 4:10 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma and Kai Uwe Broulik.
> 
> 
> Repository: kpackage
> 
> 
> Description
> ---
> 
> new job based update() function that compared to install()
> if a package with the same pluginId is already installed,
> removes the old one before installing the new one, if
> and only if the version of the new one is more recent
> 
> 
> Diffs
> -
> 
>   autotests/plasmoidpackagetest.h f730dce 
>   autotests/plasmoidpackagetest.cpp 567 
>   src/kpackage/CMakeLists.txt 3696f37 
>   src/kpackage/package.h 4ada8da 
>   src/kpackage/package.cpp 539b21a 
>   src/kpackage/packagestructure.h 9427b42 
>   src/kpackage/packagestructure.cpp 0070514 
>   src/kpackage/private/packagejob.cpp 0d2241b 
>   src/kpackage/private/packagejob_p.h 267429f 
>   src/kpackage/private/packagejobthread.cpp ca523b3 
>   src/kpackage/private/packagejobthread_p.h bf8a266 
>   src/kpackage/private/versionparser.cpp PRE-CREATION 
>   src/kpackagetool/CMakeLists.txt 78e0fb0 
> 
> Diff: https://git.reviewboard.kde.org/r/125976/diff/
> 
> 
> Testing
> ---
> 
> covered by autotests
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

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

Re: Review Request 125971: k7zip: fix memleaks, lower memory usage

[Nick Shaforostoff]

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

(Updated Nov. 6, 2015, 5:32 p.m.)


Review request for KDE Frameworks and Laurent Montel.


Changes
---

I have removed qDeleteAll(m_entryList) as it was causing one test to fail.

I have added a ~K7ZipFileEntry() destructor to delete m_buffer -> this removes 
about 1mb leaked memory during 'make test' execution, as shown by valgrind


Repository: karchive


Description
---

i couldn't find the place where the pointers contained in the member arrays are 
deleted so i have added their releasing. for this i have used qDeleteAll (you 
can search for qDeleteAll in the diff)

also i have reordered members of FileInfo to reduce its 'sizeof'
also using qvector for storing pointers is as fast as using qlist (or even 
faster) and needs less memory
also i have switched qvector acces from operator[] to .at() because it is const 
(doesn't call detach() method internally)
also i have disabled the code that was filling 'method' string because it was 
not used anywhere after


Diffs (updated)
-

  src/k7zip.cpp 321620a 
  src/karchive.cpp 0ece37c 

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


Testing
---

compiles fine


Thanks,

Nick Shaforostoff

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

Re: Review Request 125969: kinit: fix Coverity issues + small optimization

[Nick Shaforostoff]

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

(Updated Nov. 6, 2015, 4 p.m.)


Review request for KDE Frameworks and Alex Merry.


Changes
---

now displayEnvVarName_c() returns const char* and displayEnvVarName() is 
wrapping it into a QByteArray


Repository: kinit


Description
---

this patch fixes two coverity issues ranked 'outstanding':
https://scan5.coverity.com/reports.htm#v39099/p10103/fileInstanceId=82663767&defectInstanceId=24554334&mergedDefectId=258481
https://scan5.coverity.com/reports.htm#v39099/p10103/fileInstanceId=82663767&defectInstanceId=24557588&mergedDefectId=258474

and also does small string-related optimization by eliminating redundant 
mallocs done by QByteArray ctor


Diffs (updated)
-

  src/kdeinit/kinit.cpp 9e775b6 

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


Testing
---

compiles fine


Thanks,

Nick Shaforostoff

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

Re: Review Request 125965: Add declarative plugin to KHolidays

[Nick Shaforostoff]

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



src/declarative/holidayregionsmodel.cpp (line 81)
<https://git.reviewboard.kde.org/r/125965/#comment60440>

i suggest enclosing strings into QByteArrayLiteral.

see http://woboq.com/blog/qstringliteral.html for explanation and 
https://gitlab.com/pteam/pteam-qtbase/commit/05663e29d047851adb9a1ef440fb78b38ff3cc9b
 for the case when QBAL are not suitable


- Nick Shaforostoff


On Nov. 5, 2015, 7:57 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125965/
> ---
> 
> (Updated Nov. 5, 2015, 7:57 p.m.)
> 
> 
> Review request for KDE Frameworks and John Layt.
> 
> 
> Repository: kholidays
> 
> 
> Description
> ---
> 
> For now it contains just the model of holiday regions
> which will be used in the Plasma calendar events
> configuration.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt e8b7970 
>   src/CMakeLists.txt c067b6c 
>   src/declarative/CMakeLists.txt PRE-CREATION 
>   src/declarative/holidayregionsmodel.h PRE-CREATION 
>   src/declarative/holidayregionsmodel.cpp PRE-CREATION 
>   src/declarative/kholidaysdeclarativeplugin.h PRE-CREATION 
>   src/declarative/kholidaysdeclarativeplugin.cpp PRE-CREATION 
>   src/declarative/qmldir PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/125965/diff/
> 
> 
> Testing
> ---
> 
> Applet configuration contains proper list of available
> holiday regions.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

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

Re: Review Request 125969: kinit: fix Coverity issues + small optimization

[Nick Shaforostoff]

> On Nov. 5, 2015, 11:37 p.m., Alex Merry wrote:
> > src/kdeinit/kinit.cpp, line 370
> > <https://git.reviewboard.kde.org/r/125969/diff/1/?file=415537#file415537line370>
> >
> > Why call fromRawData with an explicit strlen()? 
> > QByteArray(displayEnvVarName()) will have the same effect.

fromRawData doesn't copy the data provided. the deep copy will be made later by 
operator+ into a properly sized qbytearray to store "="-ending string


- Nick


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


On Nov. 5, 2015, 10:16 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125969/
> ---
> 
> (Updated Nov. 5, 2015, 10:16 p.m.)
> 
> 
> Review request for KDE Frameworks and Alex Merry.
> 
> 
> Repository: kinit
> 
> 
> Description
> ---
> 
> this patch fixes two coverity issues ranked 'outstanding':
> https://scan5.coverity.com/reports.htm#v39099/p10103/fileInstanceId=82663767&defectInstanceId=24554334&mergedDefectId=258481
> https://scan5.coverity.com/reports.htm#v39099/p10103/fileInstanceId=82663767&defectInstanceId=24557588&mergedDefectId=258474
> 
> and also does small string-related optimization by eliminating redundant 
> mallocs done by QByteArray ctor
> 
> 
> Diffs
> -
> 
>   src/kdeinit/kinit.cpp 9e775b6 
> 
> Diff: https://git.reviewboard.kde.org/r/125969/diff/
> 
> 
> Testing
> ---
> 
> compiles fine
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

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

Review Request 125971: k7zip: fix memleaks, lower memory usage

[Nick Shaforostoff]

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

Review request for KDE Frameworks and Laurent Montel.


Repository: karchive


Description
---

i couldn't find the place where the pointers contained in the member arrays are 
deleted so i have added their releasing. for this i have used qDeleteAll (you 
can search for qDeleteAll in the diff)

also i have reordered members of FileInfo to reduce its 'sizeof'
also using qvector for storing pointers is as fast as using qlist (or even 
faster) and needs less memory
also i have switched qvector acces from operator[] to .at() because it is const 
(doesn't call detach() method internally)
also i have disabled the code that was filling 'method' string because it was 
not used anywhere after


Diffs
-

  src/k7zip.cpp 321620a 

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


Testing
---

compiles fine


Thanks,

Nick Shaforostoff

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

Review Request 125969: kinit: fix Coverity issues + small optimization

[Nick Shaforostoff]

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

Review request for KDE Frameworks and Alex Merry.


Repository: kinit


Description
---

this patch fixes two coverity issues ranked 'outstanding':
https://scan5.coverity.com/reports.htm#v39099/p10103/fileInstanceId=82663767&defectInstanceId=24554334&mergedDefectId=258481
https://scan5.coverity.com/reports.htm#v39099/p10103/fileInstanceId=82663767&defectInstanceId=24557588&mergedDefectId=258474

and also does small string-related optimization by eliminating redundant 
mallocs done by QByteArray ctor


Diffs
-

  src/kdeinit/kinit.cpp 9e775b6 

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


Testing
---

compiles fine


Thanks,

Nick Shaforostoff

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

Re: Review Request 123131: Port kio_man away from kdelibs4support

[Nick Shaforostoff]

> On March 25, 2015, 9:54 p.m., Lukáš Tinkl wrote:
> > man/kio_man.cpp, line 242
> > 
> >
> > Not correct, this returns only user configured list of languages, 
> > whereas it previously returned the full list.

please see 
https://projects.kde.org/projects/kde/kdesdk/lokalize/repository/revisions/master/entry/src/common/languagelistmodel.cpp
 for the ways to get a list of all languages (there is a KDE-way and Qt-way)


- Nick


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


On March 25, 2015, 9:17 p.m., Alex Richardson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123131/
> ---
> 
> (Updated March 25, 2015, 9:17 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma and Martin Koller.
> 
> 
> Repository: kio-extras
> 
> 
> Description
> ---
> 
> Port kio_man away from kdelibs4support
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 02ae89f2524324f758450ad368f64849e39b2f7d 
>   man/CMakeLists.txt cb4585a289d3f69b8a16429ce87bfce115d1cc27 
>   man/kio_man.cpp e8cf2d70c50c4c20adbb5a81a6213175d397b76e 
>   man/kmanpart.cpp 3af7fc182806e8b89297e8de884ce611c827e881 
>   man/man2html.cpp 3c493ba334bce890b450d7b11ab00c6e783708d4 
> 
> Diff: https://git.reviewboard.kde.org/r/123131/diff/
> 
> 
> Testing
> ---
> 
> man view in kdevelop5 works
> 
> Not sure about the `KLocale::global()->languageList();` -> 
> `QLocale().uiLanguages();` change though so I would like some feedback.
> 
> 
> Thanks,
> 
> Alex Richardson
> 
>

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

Re[2]: [ki18n] src: small qstring optimizations

[Nick Shaforostoff]

> Because otherwise, it is a very bad thing to do. There is *no* reason to ever 
> do:
> 
> QString("%1").arg(foo);

There are no overloads of QString::number having fieldWidth argument.
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re[2]: [ki18n] src: small qstring optimizations

[Nick Shaforostoff]

> On Wednesday 21 January 2015 21:32:07 Nick Shaforostoff wrote:
> > +static const QString SUBSTITUTE_ME=QStringLiteral("%1");
> 
> I'm not sure that adding global static (non-POD) objects in libraries is a 
> good idea.
> We've had many problems with that in the past (see below for details), and 
> they slow down application startup by having to create all these global 
> objects even if they are never going to be used.
> 
> PS: nowadays most commits to KDE Frameworks go via reviewboard so they can be 
> reviewed prior to being pushed.

understood. i'll create a review request for fixing this.
would a static variable inside a function be enough?
or one should just have separate non-static qstrings in each method?
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel