Review Request 120876: Forward-port "Fix and future-proof Dr Konqi security methods on Bugzilla" from kde-runtime

[Ian Wadham]

> On Oct. 29, 2014, 12:41 p.m., Thomas Lübking wrote:
> > The version check is actually on the bugzilla version (KDE_MAKE_VERSION is just a bitshifting macro) - it's the important part in the original patch ;-)
> 
> Thomas Lübking wrote:
>     To prevent future confusion, one might want to add
>     
>     #define KDE_MAKE_BUGZILLA_VERSION KDE_MAKE_VERSION
>     
>     or sth. like this.
> 
> Hrvoje Senjan wrote:
>     note to self: never blindly c/p pathes ;-)
>     still, i wonder, how will those versions get evaluated if building with kdelibs > 4.5.x ?
>     i guess i should read the patch and discussion more closely.
> 
> Thomas Lübking wrote:
>     the numbers refer to bugzilla 4.5.x - "KDE_MAKE_VERSION(a,b,c)" is just "a<<16 | b<<8 | c" (no, i don't know why it wastes 8 bits ;-)
>     You'd typically test against KDE_VERSION or directly use KDE_IS_VERSION to actually compare the runtime KDE library.
>     
>     This patch however tests against numbers it queries from the bugzilla server.
> 
> Hrvoje Senjan wrote:
>     in order not to put back in dependancy to kdelibs4support, going to try with QT_VERSION_CHECK. hopefully to have same effect
> 
> Thomas Lübking wrote:
>     Yes, has.
>     However, now it reads even more misleading.
>     
>     Maybe put
>     
>     #define MAKE_BUGZILLA_VERSION(a,b,c) (((a) << 16) | ((b) << 8) | (c))
>     
>     below the includes and use that to prevent future confusion?

Please note: there is yet another "version" involved in Dr K, besides the KDE, QT and Bugzilla versions currently causing confusion. That is the version of the software against which the bug is being reported, i.e. the version of the program that has crashed. I do not think it comes up in the present patch, but keep an eye out for it as you work further... It certainly caused me a moment or two of confusion as I was reading Dr K's code.

For improved readability and less possibility of future confusion, I suggest:
    1. Change "currentVersion" to currentBugzillaVersion".
    2. The macro is just a data conversion: it does not "make" a Bugzilla version. So change its name from MAKE_BUGZILLA_VERSION to INT_BUGZILLA_VERSION or, if you like traditional Unix naming, BUGZILLA_VERSION_TOINT.
    3. Bring the macro definition inside setFeaturesForVersion(): it is (deliberately) not used anywhere else.

The version tests would then read like "if (currentBugzillaVersion >= INT_BUGZILLA_VERSION(4, 4, 3))".


- Ian


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


On Oct. 29, 2014, 8:41 p.m., Hrvoje Senjan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120876/
> -----------------------------------------------------------
> 
> (Updated Oct. 29, 2014, 8:41 p.m.)
> 
> 
> Review request for Plasma, Ben Cooksley, Ian Wadham, and Thomas Lübking.
> 
> 
> Bugs: 337742
>     https://bugs.kde.org/show_bug.cgi?id=337742
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> -------
> 
> discussion was in https://git.reviewboard.kde.org/r/120431/
> removed the version checks, as we know we have kdelibs >= 4.5 ;-)
> 
> 
> Diffs
> -----
> 
>   drkonqi/bugzillaintegration/bugzillalib.h 570169b 
>   drkonqi/bugzillaintegration/bugzillalib.cpp 8fd8399 
>   drkonqi/bugzillaintegration/reportassistantpages_bugzilla.h 50cf05f 
>   drkonqi/bugzillaintegration/reportassistantpages_bugzilla.cpp 5a6096f 
> 
> Diff: https://git.reviewboard.kde.org/r/120876/diff/
> 
> 
> Testing
> -------
> 
> builds, succesfully reported bug via patched DrKonqi, wasn't able to do so before.
> 
> 
> Thanks,
> 
> Hrvoje Senjan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20141030/448ca0f5/attachment.html>