Review Request 126440: Fix some Clazy warnings in kinit.

[Andrey Cygankov]

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

Review request for KDE Frameworks and Aleix Pol Gonzalez.


Repository: kinit


Description
---

Fix some Clazy warnings in kinit:
- unneeded heap allocation with QString
- qEnvironmentVariableIsEmpty() non-usage
- isEmpty() non-usage


Diffs
-

  src/kdeinit/kinit.cpp a18008a 
  src/klauncher/klauncher.h e155f72 
  src/klauncher/klauncher.cpp 8b3d343 
  src/klauncher/klauncher_main.cpp 432e19a 
  src/wrapper.cpp 95b7ec2 

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


Testing
---

Compiling without errors.
Automated tests pass.


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 126409: Fix use-after-free in .desktop file parser

[Michael Pyne]

> On Dec. 18, 2015, 3:17 a.m., Alex Richardson wrote:
> > src/lib/plugin/desktopfileparser.cpp, line 346
> > 
> >
> > `defs << *def // This must...` instead? And then the `defs << *def` in 
> > the else block? Would remove the need for the temporary variable 
> > declaration at the top.
> > 
> > Switching the if/else would also make this easier to understand.
> 
> Michael Pyne wrote:
> I prefer to factor loop invariants (such as updated defs here) out of 
> if/else blocks, which is why I used temporary, but whichever fix is used will 
> be pretty obvious here so I've submitted a revision.

Any objections to the revised patch?


- Michael


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


On Dec. 18, 2015, 3:44 a.m., Michael Pyne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126409/
> ---
> 
> (Updated Dec. 18, 2015, 3:44 a.m.)
> 
> 
> Review request for KDE Frameworks and Alex Richardson.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Patch to fix a use-after-free issue in 
> kcoreaddons/src/lib/plugin/desktopfileparser.cpp, noted by Coverity (CID 
> 1336157)
> 
> The issue is that if `defs << *def` happens after the call to 
> s_serviceTypes->insert(), the `*def` might already have been deleted by 
> QCache. Attached patch fixes by making the copy before the call to insert().
> 
> Really, it's probably better to not use QCache here if we can avoid it, but 
> I'm not sure enough of the code to go that route. But if this is something 
> that's only run once or twice anyways then there's no real need to use QCache 
> instead of QMap, I would think.
> 
> 
> Diffs
> -
> 
>   src/lib/plugin/desktopfileparser.cpp 06a4a1d 
> 
> Diff: https://git.reviewboard.kde.org/r/126409/diff/
> 
> 
> Testing
> ---
> 
> desktoptojsontest still passes, code compiles.
> 
> 
> Thanks,
> 
> Michael Pyne
> 
>

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

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

[Andrey Cygankov]

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


Hey, when my patch will be reviewed?

- Andrey Cygankov


On Дек. 18, 2015, 7:32 п.п., Andrey Cygankov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126392/
> ---
> 
> (Updated Дек. 18, 2015, 7:32 п.п.)
> 
> 
> 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
> -
> 
>   autotests/ktexttohtmltest.cpp 81c3fc0 
>   src/lib/io/kbackup.h ead2de5 
>   src/lib/io/kbackup.cpp ec8b8b2 
>   src/lib/io/kdirwatch.cpp 34b32b7 
>   src/lib/io/kdirwatch_p.h 4483961 
>   src/lib/io/kmessage.cpp 305318c 
>   src/lib/io/kprocess.cpp 1df61c0 
>   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 126392: Fix some Clazy warnings in kcoreaddons

[Michael Pyne]

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

Ship it!


Revised patch looks good here.

- Michael Pyne


On Dec. 18, 2015, 7:32 p.m., Andrey Cygankov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126392/
> ---
> 
> (Updated Dec. 18, 2015, 7:32 p.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
> -
> 
>   autotests/ktexttohtmltest.cpp 81c3fc0 
>   src/lib/io/kbackup.h ead2de5 
>   src/lib/io/kbackup.cpp ec8b8b2 
>   src/lib/io/kdirwatch.cpp 34b32b7 
>   src/lib/io/kdirwatch_p.h 4483961 
>   src/lib/io/kmessage.cpp 305318c 
>   src/lib/io/kprocess.cpp 1df61c0 
>   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 126423: KPluginSelector::addPlugins: do not assert if 'config' parameter is null

[Michael Pyne]

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

Ship it!


Ship It!

- Michael Pyne


On Dec. 19, 2015, 8:34 a.m., Jonathan Marten wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126423/
> ---
> 
> (Updated Dec. 19, 2015, 8:34 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Bugs: 352471
> https://bugs.kde.org/show_bug.cgi?id=352471
> 
> 
> Repository: kcmutils
> 
> 
> Description
> ---
> 
> The API documentation says that the 4-argument form of 
> KPluginSelector::addPlugins() can be called with the last 'config' parameter 
> as null (the default), meaning the standard application config file.  
> However, there seems to be a misplaced assert which means that this will not 
> be accepted.
> 
> 
> Diffs
> -
> 
>   src/kpluginselector.cpp 34a7897 
> 
> Diff: https://git.reviewboard.kde.org/r/126423/diff/
> 
> 
> Testing
> ---
> 
> Built kcmutils with this change, checked with Konqueror's "Configure 
> Extensions" dialogue as described in the bug report.  Observed assert/crash 
> without this change, and correct operation with it.
> 
> 
> Thanks,
> 
> Jonathan Marten
> 
>

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

Re: Review Request 126440: Fix some Clazy warnings in kinit.

[Michael Pyne]

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

Ship it!


Ship It!

- Michael Pyne


On Dec. 20, 2015, 10:44 p.m., Andrey Cygankov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126440/
> ---
> 
> (Updated Dec. 20, 2015, 10:44 p.m.)
> 
> 
> Review request for KDE Frameworks and Aleix Pol Gonzalez.
> 
> 
> Repository: kinit
> 
> 
> Description
> ---
> 
> Fix some Clazy warnings in kinit:
> - unneeded heap allocation with QString
> - qEnvironmentVariableIsEmpty() non-usage
> - isEmpty() non-usage
> 
> 
> Diffs
> -
> 
>   src/kdeinit/kinit.cpp a18008a 
>   src/klauncher/klauncher.h e155f72 
>   src/klauncher/klauncher.cpp 8b3d343 
>   src/klauncher/klauncher_main.cpp 432e19a 
>   src/wrapper.cpp 95b7ec2 
> 
> Diff: https://git.reviewboard.kde.org/r/126440/diff/
> 
> 
> Testing
> ---
> 
> Compiling without errors.
> Automated tests pass.
> 
> 
> 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 126324: [MSWin/OS X] save and restore window geometry instead of only size (WIP/Suggestion)

[René J . V . Bertin]

> On Dec. 17, 2015, 5:16 p.m., Martin Gräßlin wrote:
> > src/gui/kwindowconfig.h, lines 38-39
> > 
> >
> > That doesn't match the method name. It's saveWindowSize, not 
> > saveWindowGeometry. It's highly unexpected that saveWindowSize saves the 
> > position.
> > 
> > If you want that: please introduce a new saveWindowGeometry method.
> 
> René J.V. Bertin wrote:
> I was afraid someone was going to say that, which is why I tried to argue 
> that it's highly unexpected from a user viewpoint that only window size is 
> saved and not position. How often would it happen that a developer is "highly 
> surprised" in a *negative* way that window size AND position are restored on 
> a platform where this is the default behaviour?
> 
> I have nothing against introducing a pair of new methods, but how is that 
> supposed to be done in transparent fashion? I do have a lot against a need to 
> change all dependent software to call those methods (maintenance burden and 
> all that).
> 
> Counter proposal: replace save/restoreWindowSize with 
> save/restoreWindowGeometry everywhere, with a platform-specific 
> interpretation of what exactly geometry encompasses. Much less surprise 
> there, just a bit more need to read the documentation. Are these functions 
> ever called intentionally outside of what I suppose is a more or less 
> automatic feature that takes care of restoring window, erm, layout (saving is 
> clearly automatic).
> 
> René J.V. Bertin wrote:
> Just to be clear: if I am going to introduce restore/saveWindowGeometry 
> methods they'll replace the WindowSize variants on OS X or at least those 
> will then use a different KConfig key to avoid conflicts. 
> I'd also be dropping the MS Windows part of the patch (as this is not a 
> decision I want to make for a platform I don't use).
> 
> But please consider this: that KConfig key has been called `geometry` for 
> a long time. Where exactly is the surprise, that restore/saveWindowSize never 
> did what the key they operate with suggests, or that they have always been 
> using an inaptly named key? For me the answer is straightforward and based on 
> what users will expect...
> 
> Martin Gräßlin wrote:
> I leave it to the maintainers. On API I maintain I would say no to 
> something changing the semantics like that.
> 
> René J.V. Bertin wrote:
> As I just wrote in reply to a message from Valorie, I have absolutely no 
> issue with maintaining methods for saving and restoring only window size, for 
> code that somehow requires that. I'd guess that such code would probably 
> enforce the intended window position itself *after* restoring window size 
> (because that operation *can* affect window position), but in the end that's 
> (indeed) up to the code's developers to decide.
> 
> IOW, I'm perfectly willing to discuss a better solution in which the 
> burden to ensure that window save/restore works as "natively" as possible on 
> each platform is shared. The best way to do that is of course to have a 
> single pair of methods that have platform-specific implementations.
> 
> As far as I'm concerned such a solution might even be prepared completely 
> in KConfig/gui before changes are made everywhere else to deploy that new 
> solution. In that case I would for instance run temporary local (MacPorts) 
> patches that replace saveWindowSize/restoreWindowSize with wrappers for 
> saveWindowGeometry/restoreWindowGeometry.
> 
> Side-observation: OS X (Cocoa) provides a `[NSWindow 
> setFrameAutosaveName:]` method, i.e. it avoids reference to specifics like 
> size or geometry completely.
> 
> That method also provides another thought that could be taken into 
> consideration if it is decided to evolve this part of the frameworks, 
> something I'd be interested in collaborating on. Currently, there is no 
> support for saving and restoring multiple windows per application. That may 
> be more or less sufficient when applications always follow a MDI approach, 
> but even if they do that still doesn't make them applications that are active 
> only in a single instance. Example: KDevelop. One might expect that opening a 
> given, pre-existing session (collection of open projects) restores the main 
> window geometry (size and/or position) that used previously for that session, 
> rather than the geometry used by whatever KDevelop session was run last. On 
> OS X that would be done with something like `[NSWindow 
> setFrameautosaveName:[window representedFile]]`, where `[NSWindow 
> representedFile]` corresponds to `QWindow::filePath` (but AFAICS those are 
> not coupled in Qt5).
> 
> I already had a quick look, but realised I don't know if the KConfig 
> mechanism has facilities to handle cleanup of stale/obsolete key/value 
> entries.
> 
> David Faure wrote:
> Note that most apps use this 

Re: [Kde-pim] kdesrc-build setup for kdepim

[Ovidiu-Florin BOGDAN]

> I've been strugling since wednesday to build kdepim with KSB. I think I've 
> managed to sort out most dependencies. Only kdepim-runtime and kdepin dont 
> build. Reason is I don't have kross *.16 (kubuntu comes with *.15. I've tried 
> to build kross with KSB but it fails with:
> [ 64%] Automatic moc for target kf5kross
> [ 64%] Built target kf5kross_automoc
> Linking CXX executable kf5kross
> /home/ovidiu/Qt/5.5/gcc_64/lib/libQt5Script.so.5.5.1: referinţă nedefinită 
> către `QMetaObject::Connection::isConnected_helper() const'
> collect2: error: ld returned 1 exit status
> src/console/CMakeFiles/kf5kross.dir/build.make:110: recipe for target 
> 'src/console/kf5kross' failed
> make[2]: *** [src/console/kf5kross] Error 1
> CMakeFiles/Makefile2:519: recipe for target 
> 'src/console/CMakeFiles/kf5kross.dir/all' failed
> make[1]: *** [src/console/CMakeFiles/kf5kross.dir/all] Error 2
> Makefile:126: recipe for target 'all' failed
> make: *** [all] Error 2
I've finnaly solved this. It apears that I had Qt 5.5.0 instead of 5.5.1.


> P.S. How's the dependency fixing going?
I've had to manually update many of the projects that are dependencies but they 
are not marked as such in KSB and also bring in new ones that I didn't have.
If it's useful, I can go through the command history and gather the ones that I 
had to bring in or update because they were not marked as dependencies. Let me 
know if this is needed.

> On my first run KSB cried that I should change *module* to *options*, so I 
> did, and it runs now.
I've reverted this and updated the source of KSB. It's fine now.


*Ovidiu-Florin Bogdan*
GeekAliens.com[1]
Kubuntu Romania Representative[2]
_Kubuntu Council Member_


[1] http://ovidiu.geekaliens.com
[2] http://kubuntu.org


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