D27109: Fix duplicate results for sessions, simplify and optimize

[Christoph Cullmann]

This revision was automatically updated to reflect the committed changes.
Closed by commit R114:2c832dbdd087: Fix duplicate results for sessions, 
simplify and optimize (authored by alex, committed by cullmann).

REPOSITORY
  R114 Plasma Addons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27109?vs=74875=75775

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

AFFECTED FILES
  runners/katesessions/CMakeLists.txt
  runners/katesessions/katesessions.cpp
  runners/katesessions/katesessions.h

To: alex, broulik, davidedmundson, ngraham, #plasma, #kate, cullmann
Cc: cullmann, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27109: Fix duplicate results for sessions, simplify and optimize

[Christoph Cullmann]

cullmann added a comment.


  Upsa ;) missed that this is in plasma addons, not in the session launcher 
applet we have in our repo, ignore my comment about the invent stuff ;=)

REPOSITORY
  R114 Plasma Addons

BRANCH
  katesession_improvements (branched from master)

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

To: alex, broulik, davidedmundson, ngraham, #plasma, #kate, cullmann
Cc: cullmann, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27109: Fix duplicate results for sessions, simplify and optimize

[Christoph Cullmann]

cullmann added a comment.


  Btw., for Kate, we would prefer merge requests on invent.kde.org/kde/kate, 
but I will just apply this via arc.

REPOSITORY
  R114 Plasma Addons

BRANCH
  katesession_improvements (branched from master)

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

To: alex, broulik, davidedmundson, ngraham, #plasma, #kate, cullmann
Cc: cullmann, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27109: Fix duplicate results for sessions, simplify and optimize

[Christoph Cullmann]

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


  Improved output seems fine for me, thanks for taking care.

REPOSITORY
  R114 Plasma Addons

BRANCH
  katesession_improvements (branched from master)

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

To: alex, broulik, davidedmundson, ngraham, #plasma, #kate, cullmann
Cc: cullmann, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D24373: [KCM] Limit scale factor increment to 6.25% on X11

[Christoph Cullmann]

cullmann added a comment.


  On Wayland one seems to always scale up to the next full factor (e.g. 2) and 
then scale down the rendered stuff via gl to the selected factor (e.g 1.5).
  
  That leads to other issues but doesn't let rounding stuff creep into the 
rendering via Qt.
  
  For non IEEE stuff: perhaps that will be available in the future, but at the 
moment all really available RISC-V ISAs provide just IEEE, too.
  And staying in some 1/(2^x) range won't hurt any alternative approach I 
assume.

REPOSITORY
  R104 KScreen

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

To: ngraham, #plasma, romangg, mart
Cc: MayeulC, cullmann, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24283: Add clang format file

[Christoph Cullmann]

cullmann added a comment.


  There will anyways be one "big" change in the history, but normally git will 
be able to handle that well by ignoring space changes for blame or diff.
  I don't think without the aligning the change will be much smaller.

REPOSITORY
  R120 Plasma Workspace

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

To: davidedmundson
Cc: zzag, cullmann, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24283: Add clang format file

[Christoph Cullmann]

cullmann added inline comments.

INLINE COMMENTS

> zzag wrote in .clang-format:83
> s/coding/coding style/

Fixed that in D24568 

REPOSITORY
  R120 Plasma Workspace

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

To: davidedmundson
Cc: zzag, cullmann, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24283: Add clang format file

[Christoph Cullmann]

cullmann added a comment.


  This is just a base file for discussion.
  Stuff like the mentioned case indentation can be adjusted to be frameworks 
conform.
  For the alignment of comments, I am not sure, it makes the stuff a lot more 
readable in many cases and after the initial re-format you don't have a lot of 
jitter normally.

REPOSITORY
  R120 Plasma Workspace

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

To: davidedmundson
Cc: zzag, cullmann, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24283: Add clang format file

[Christoph Cullmann]

cullmann added a comment.


  See D24568  for the draft of an ECM 
module that applies the Kate/KTextEditor/Artikulate style.
  Is the same file as here, with sorting of includes, but with column limit.

REPOSITORY
  R120 Plasma Workspace

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

To: davidedmundson
Cc: cullmann, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24373: [KCM] Limit scale factor increment to 0.0625 on X11

[Christoph Cullmann]

cullmann added a comment.


  I would just reformulate
  
  "Scale factors are limited to multiples of 6.25% on X11 to minimize visual 
glitches in applications."
  
  to
  
  "Scale factors are limited to multiples of 6.25% to minimize visual glitches 
in applications."
  
  I think adding X11 only makes this harder to understand for the users.

REPOSITORY
  R104 KScreen

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

To: ngraham, #plasma, romangg
Cc: cullmann, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a spinbox

[Christoph Cullmann]

cullmann added a comment.


  Ok ,works for me, too. Sorry for the noise! And thanks for the explanation!
  I think this is already much better than before, the details of the last 
finesse with the minimal step can be discussed elsewhere.

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg, ndavis
Cc: dhaumann, davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, 
LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, 
alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a spinbox

[Christoph Cullmann]

cullmann added a comment.


  Ok, that explains why all my tries fail...
  I thought the hint above about looking for kcm_screen in the system was just 
if I have mixed up some stuff, but I always thought if the QT_PLUGIN_PATH is 
properly set like with prefix.sh the local plugins will be prefered.
  Will try at home.

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg, ndavis
Cc: dhaumann, davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, 
LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, 
alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a spinbox

[Christoph Cullmann]

cullmann added a comment.


  Ok, but given I have a system wide installed kcm anyways, from the distro, is 
there a way to use the own I have in my user local prefix?
  That explains why I am too dumb to try the fractional scaling on Wayland with 
my installed master version, too.

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg, ndavis
Cc: dhaumann, davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, 
LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, 
alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a spinbox

[Christoph Cullmann]

cullmann added a comment.


  Or I am maximal confused ;=)
  
  If I compile a master kscreen, have sourced the prefix.sh, shall I see the 
correct stuff in e.g. the kcm_testapp in bin of the kscreen build dir?
  
  If not, I am sorry, could somebody help me out how to test this properly?

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg, ndavis
Cc: dhaumann, davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, 
LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, 
alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a spinbox

[Christoph Cullmann]

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


  Besides, I think you missed half of the code, as for me, I still get a 
different widget here, as for my setup the OutputPanel.qml is used, not 
Panel.qml.
  
  (as I have two displays that can be configured separately)
  
  Could you fix that, independent of the 0.05.. issue?
  I agree that we can do an extra patch for that.

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg, ndavis
Cc: dhaumann, davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, 
LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, 
alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a spinbox

[Christoph Cullmann]

cullmann added a comment.


  I don't get why we want to introduce artifacts we can very simple avoid by 
using a different base for the steps.
  Is 0.0625 really that much worse than 0.05?

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg, ndavis
Cc: dhaumann, davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, 
LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, 
alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a spinbox

[Christoph Cullmann]

cullmann added a comment.


  To illustrate that, just try, with master of KTextEditor:
  
  export QT_SCALE_FACTOR=1.125
  
  kwrite with some file => select 100 lines, zoom font in and out => I get no 
artifacts
  
  export QT_SCALE_FACTOR=1.0625
  
  > same
  ==
  
  export QT_SCALE_FACTOR=1.1
  
  > artifacts each X lines
  

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg, ndavis
Cc: dhaumann, davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, 
LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, 
alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a spinbox

[Christoph Cullmann]

cullmann added a comment.


  Hmm, could we perhaps adjust the minimal step to 0,0625.
  That is 1/16 and with that, I never get any artifacts with KWrite in current 
master, as it nicely sums up.
  Then you don't need to round at all for output.

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg, ndavis
Cc: dhaumann, davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, 
LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, 
alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a spinbox

[Christoph Cullmann]

cullmann added a comment.


  I like that UI, too.
  For Wayland: Can somebody tell me how I can try there some fractional scaling?

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg, ndavis
Cc: dhaumann, davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, 
LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, 
alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a semi-hidden spinbox

[Christoph Cullmann]

cullmann added a comment.


  Even with my hacks for https://bugreports.qt.io/browse/QTBUG-66036 in current 
KTextEditor master (see https://bugs.kde.org/show_bug.cgi?id=390451) you will 
still get after every xx lines some stray artifact in some cases during 
selection if you have some 1.1 scaling.
  That still seems to be a remaining issue with selection painting. Not 
happening with proper scaling of 1.25 or 1.5 as nothing adds up multiplication.

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg
Cc: dhaumann, davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, 
LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, 
alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a semi-hidden spinbox

[Christoph Cullmann]

cullmann added a comment.


  Ok, I wasted one evening on the rendering artifacts stuff.
  
  https://bugreports.qt.io/browse/QTBUG-66036
  
  I have now ugly workarounds in KTextEditor:
  
  1. no anti-aliasing, else fillRect misses to fill 1 pixel at the borders by 
rounding errors
  2. adjust the clip rect by one logical pixel to not miss 1 real device pixel 
sometimes

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg
Cc: davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a semi-hidden spinbox

[Christoph Cullmann]

cullmann added a comment.


  I got some more insight into https://bugreports.qt.io/browse/QTBUG-66036
  If one turns off anti-aliasing, in KTextEditor most of the evil artifacts 
disappear, but not all.

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg
Cc: davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a semi-hidden spinbox

[Christoph Cullmann]

cullmann added a comment.


  I will investigate the https://bugreports.qt.io/browse/QTBUG-66036 bug again, 
too :/
  Even with the rounding issues one gets over larger multiplications, it 
shouldn't be that buggy for short ones.

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg
Cc: davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a semi-hidden spinbox

[Christoph Cullmann]

cullmann added a comment.


  Ok, tried Wayland now on my HiDPI setup.
  In Plasma's KCM I only have Scale 1x or 2x and that works ok, as one would 
think.
  But I see no way to set any more fine-grained scale to check if the scaling 
is nice for other values.
  The slider is for me only there for X11.

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg
Cc: davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a semi-hidden spinbox

[Christoph Cullmann]

cullmann added a comment.


  In D24321#540437 , @romangg wrote:
  
  > In D24321#540433 , @cullmann 
wrote:
  >
  > > ;=) I talked only about the Qt HiDPI scaling code paths.
  >
  >
  > In this case I recommend studying the bug report I linked. There is also a 
patch attempt in Qt but it got closed for some reason.
  
  
  You can't patch that issues in Qt if e.g. your application relies on facts 
like you paint 10 lines and you don't get some rounding errors all the time ;)
  
  > 
  > 
  >> For any other scaling I don't care, as that should be application 
transparent, like you say.
  >>  But if Wayland really just scales up the stuff via GL fonts will look 
terrible.
  > 
  > They are scaled down via Gl to match the fractional part of the scaling 
value. Applications provide pre-scaled buffers of integer factors 2, 3, 4 and 
so on.
  
  Hmm, but that means you still set QT_SCALE_ to some non-fractional thing 
like 2, 3, 4 and the only fine adjust, that makes sense.
  Thought for me it did look like the small scale window was up-scaled in my 
try with the 2x combobox field.
  
  > 
  > 
  >> And no, its non-trivial to fix there any artifacts for strange scales and 
it makes there computation wise a very large different if the scale factor is 
1.2 or 1.25, as the later is something you can sanely compute things with ;)
  >>  Therefore I really would like to have only some larger scaling steps set 
for the QT_SCALE_FACTOR... stuff and for the smaller things just some font 
adjustments.
  > 
  > Don't see a reason for that. Never heard about necessity in Gl to only 
calculate with certain floating values for best image quality. But if you can 
provide some source for that I would be interested in studying that. Can't say 
I see problems here in a Wayland session though, currently using a scaling 
factor of 1.8.
  
  The issue is that you transform individual elements.
  
  e.g. if you have some list of X graphical elements with height "virtual" 
pixel 10. If you scale them by 1.1, you get not "real pixel" 11, you get some 
11.00100101010 something. If you later compute things like "10 * element size", 
you not get 110 pixel but again some weired stuff ;=)
  
  
https://www.exploringbinary.com/why-0-point-1-does-not-exist-in-floating-point/
  
  If you apply that as transform on the full texture, I can agree that it 
shouldn't be a real issue.
  But if it is applied as QT_SCALE... you will have the issue of rounding 
artifacts for the individual widgets/layout members that then sum up in weired 
ways. 
  You get already some artifacts with 1/2 or 1/4, but at least you can multiply 
that sanely (as long as your floating point numbers don't get enourmous).

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg
Cc: davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a semi-hidden spinbox

[Christoph Cullmann]

cullmann added a comment.


  Hmm, I just tried here a Wayland session, and yes, the KCM has there just 
some 1x/2x combobox and my fonts are maximal ugly compared to some export 
QT_SCALE_FACTOR=2 + starting a Qt application (on Wayland).
  But I must try that at home on real HiDPI screens, perhaps my test is 
suboptimal here.

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg
Cc: davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a semi-hidden spinbox

[Christoph Cullmann]

cullmann added a comment.


  ;=) I talked only about the Qt HiDPI scaling code paths.
  
  For any other scaling I don't care, as that should be application 
transparent, like you say.
  But if Wayland really just scales up the stuff via GL fonts will look 
terrible.
  
  And no, its non-trivial to fix there any artifacts for strange scales and it 
makes there computation wise a very large different if the scale factor is 1.2 
or 1.25, as the later is something you can sanely compute things with ;)
  Therefore I really would like to have only some larger scaling steps set for 
the QT_SCALE_FACTOR... stuff and for the smaller things just some font 
adjustments.

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg
Cc: davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a semi-hidden spinbox

[Christoph Cullmann]

cullmann added a comment.


  In D24321#540218 , @davidedmundson 
wrote:
  
  > >   For small 0.1 changes: Just change your font size, we did that always 
for the old non-hidpi world were displays did differ a bit in the DPI, that 
works without any glitches
  >
  > I even used to do this in the new high DPI world. Before Qt had fractional 
scaling we had the slider it would adjust the font size until you reached 1.75 
then it would change the Qt scale too. It worked fine.
  
  
  Perhaps such a mixed approach might even make more sense.
  
  As said, any scaling like 1.2 will not work with most of our applications, 
see above Konsole.
  
  With "a lot" of work, stuff like 1.25 or 1.5 will work like in KTextEditor by 
adjusting the fonts internally a bit to avoid sub-pixel jitter per line.
  
  I think we shall really avoid to expose bad scale factors to users.

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg
Cc: davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a semi-hidden spinbox

[Christoph Cullmann]

cullmann added a comment.


  In D24321#540056 , @davidedmundson 
wrote:
  
  > > it would go into Plasma 5.17. :)
  >
  > There's still a feature freeze.
  
  
  At least moving away from 0.1 increases is actually a bugfix. Look at my 
konsole screenshot, similar artifacts can be created with most of our 
applications.
  (sadly still with 0.25/0.5, too, but at least not that often and you then can 
try to fix that by choosing some better font...)

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg
Cc: davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D24321: [KCM] Scale more coarsely with the slider, but more finely with a semi-hidden spinbox

[Christoph Cullmann]

cullmann added a comment.


  Hi, I thought about this a bit more ;)
  
  I really think we shall limit the minimal increase to 0.25, without any 
advanced field. Perhaps the range should be larger, like 1-4, if some people 
get "really" high DPI screens.
  
  Rational:
  
  1. For small 0.1 changes: Just change your font size, we did that always for 
the old non-hidpi world were displays did differ a bit in the DPI, that works 
without any glitches
  
  2. Anything below 0.25 will just break half of our applications. For 
KTextEditor I did some hack in master that might avoid artifacts if you only 
scale with stuff like .25 or .5, it will still break with small stuff and most 
of our applications look horrible with small scalings, look at attached 
screenshot fo 1.1, do we really want to advertise that?
  
  F7481758: screenshot.png 
  
  Qt 5.14 will even have "opt-out" of fractional scaling and perhaps we even 
need to do that for some applications...

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg
Cc: davidedmundson, ouwerkerk, GB_2, ndavis, cullmann, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D24321: [KCM] Scale more grossly with the slider, but more finely with a semi-hidden spinbox

[Christoph Cullmann]

cullmann added a comment.


  Btw., I think GNOME is more clever than us, too, they allow just sane 
scalings in the UI:
  
  https://www.omgubuntu.co.uk/2019/06/enable-fractional-scaling-ubuntu-19-04

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg
Cc: GB_2, ndavis, cullmann, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D24321: [KCM] Scale more grossly with the slider, but more finely with a semi-hidden spinbox

[Christoph Cullmann]

cullmann added a comment.


  And the custom scale factor dialog really should have some warning text like 
on Windows ;=)
  
  see e.g.
  
  
https://www.pcworld.com/article/2953978/use-windows-10s-individual-display-scaling-to-perfect-your-multi-monitor-setup.html
  
  (custom scale factor not recommended)

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg
Cc: KonqiDragon, GB_2, ndavis, cullmann, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D24321: [KCM] Scale more grossly with the slider, but more finely with a semi-hidden spinbox

[Christoph Cullmann]

cullmann added a comment.


  In D24321#540014 , @ngraham wrote:
  
  > In D24321#540011 , @cullmann 
wrote:
  >
  > > 0.05 is no good idea either ;)
  > >  Just like 0.1 that is no 1/2, 1/4, 1/8, ...
  > >  If we really need something finer than 0.25, could we stick with 1/8 or 
1/16?
  >
  >
  > I don't want to lock people out of the finer scale factor numbers. 
Depending on your screen DPI and size, you may want to set and be quite happy 
with a weird scale factor like 1.65x. The idea here is to expose the more 
common and bug-free scale factors in the main UI, while letting experts and 
risk-takers set whatever they want. Maybe I should add a warning in the dialog 
that people who set a weird value there are on their own?
  
  
  That is fine, but please use some step width that is a proper float.
  Just use 1/16, that is 0,0625.
  0.05 will just lead to the same issues as 0.1, it looks nice in the input 
field but is no floating pointer number you can calculate sanely with.

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg
Cc: GB_2, ndavis, cullmann, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D24321: [KCM] Scale more grossly with the slider, but more finely with a semi-hidden spinbox

[Christoph Cullmann]

cullmann added a comment.


  0.05 is no good idea either ;)
  Just like 0.1 that is no 1/2, 1/4, 1/8, ...
  If we really need something finer than 0.25, could we stick with 1/8 or 1/16?

REPOSITORY
  R104 KScreen

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

To: ngraham, #vdg, #plasma, romangg
Cc: cullmann, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D22630: Slow down the busy indicator's rotation speed

[Christoph Cullmann]

cullmann accepted this revision.
cullmann added a comment.


  Jup, looks less iritating.

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

BRANCH
  slower-busy-indicator

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

To: ngraham, #vdg, filipf, cullmann
Cc: filipf, plasma-devel, cullmann, LeGast00n, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D22525: kioclient: Don't convert `:x:y` to `?line=x=y` for URLs starting with remote schemes.

[Christoph Cullmann]

cullmann added a comment.


  Thanks!

REPOSITORY
  R126 KDE CLI Utilities

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

To: arrowd, #frameworks, dfaure
Cc: cullmann, wbauer, kwrite-devel, dfaure, cfeck, plasma-devel, #frameworks, 
LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

Re: KInit - Current state and benchmarks

[Christoph Cullmann]

Hi,

On 2019-06-17 11:56, David Edmundson wrote:

From API.kde.org:
Using kdeinit to launch KDE applications makes starting a typical KDE 
applications 2.5 times faster (100ms instead of 250ms on a P-III 500)


Certainly sounds like a good thing.

===The current State===

==Plasma==
* Apps launched from the plasma menu skip klauncher and therefore
kinit. This was done due to the API for KRun blocking somewhere
(Though I don't remember details)

This seems like something easily fixable if we tried.

* Processes launched on bootup (with the exception of kcminit,
ksmserver and kded5) skip kinit. This was due to a deadlock in
klauncher and ksmserver at the time when apps autostart moved from
frameworks.

This deadlock is since resolved in my ksmserver splitting branches.

* But Plasma apps launched from the desktop do go through klauncher
(and therefore kinit)! So we're not even consistent.

==Apps==
* The number of applications linking kinit properly under KF5 is in an
equally sorry state.
Dolphin does, but even core applications like Okular and Kate don't.
I stopped using it I guess to be consistent between all platforms 
shipping Kate.
(beside, if you profile the Kate startup, all things are > the stuff 
kdeinit saves)




See
$ find -name 'CMakeLists.txt' -print0 | xargs -0 grep
'kf5_add_kdeinit_executable'
for a local list.

* It's also incompatible with flatpak/snaps/appimage, which is a rising 
trend


==Other==
Kinit is still also used for spawning KIO slaves.

===Is it still useful?===
We're not using it properly and we need to do something. Either fix it
or start to phase it out officially.

Since the time of writing kinit Qt has changed a lot. linking has
changed a bit. CPUs have changed a lot and Hard Disks have changed a
lot.

So I wrote a benchmark tool to see if the initial speed boost claim is
relevant: kde:scratch/davidedmundson/inittimer

My test does the following:
 - creates a dummy (headless) wayland server
 - spawns an app using either QProcess or sending a DBus message to 
KLauncher

 - times how long it takes for the first window to appear, timing the
more important real world metric and one that includes all the
factors.

Results:
(Showing the median out of 5 runs on a mid range SSD desktop)

Dolphin QProcess: 348
Dolphin Kinit: 332

KCalc   QProcess:  242
KCalc   KInit: 232

Plasmoidviewer (patched) QProces: 622
Plasmoidviewer (patched) KInit: 591

KWrite  QProcess: 391
KWrite  Kinit: 390
(unsurprisingly similar as kwrite does not have a kdeinit exec, I
included it as it shows the others aren't false positives)

===What about memory?===

Good question. It needs a similar investigation by someone who
understands memory...

===Conclusion===

My test implies there /is/ a genuine saving with kinit !
However it's far from the claimed 2.5 times faster, it is less than
1.1 times faster. Saving up to 30ms.


my opinion: it's not worh the hassle.

Greetings
Christoph

--
Ignorance is bliss...
https://cullmann.io | https://kate-editor.org

Re: CI system maintainability

[Dr.-Ing. Christoph Cullmann]

Hi,

> Hi,
> 
> On Thu, 28 Mar 2019 at 15:21, Kevin Ottens  wrote:
> 
>> Hello,
>>
>> On Thursday, 28 March 2019 14:33:59 CET laurent Montel wrote:
>> > I am against to force mandatory review, as it will create a lot of lose
>> of
>> > time,
>>
>> As I said, unpopular.
>>
> 
> I don't get why mandatory code reviews are so unpopular.
> 
> I don't care if you lose time. I don't want the guys building my house to
> cut corners mixing my concrete because it's going to save time. Why are you
> in such a massive hurry to make changes to software which for example holds
> access to my Google Account password? In fact, the very fact that you make
> this argument makes me wonder if I'm running trustable code on my computer
> at all, because apparently doing it quickly is far more important than
> doing it right.
> 
> As a user, I simply do not want unreviewed crap running on my computer.
> Yes, crap, because no software engineer writes bug-free code all the time,
> and if you're so overconfident that you don't need reviews on even your
> one-liners, you're probably too overconfident to be writing good code
> anyway, so I'm going to operate on the presumption that if the code hasn't
> had more than one pair of eyeballs ever looking at it, it's crap.
> 
> As a developer, I know that even one-liners, and especially one-liners, the
> sort where you think "meh, this is a tiny little thing, I don't have to be
> careful" are the ones that have the most dangerous typos and unintended
> bugs. Reviews catch that.
> 
> In a project like PIM, if the code hasn't been through review, which
> independent party do I trust to verify that you're not, for example,
> leaking my Google password to some world-readable tempfile? Do you really
> expect every user to read the entire codebase for themselves and make sure
> that's not being done? The whole point of having all the code out in the
> open for independent audit purposes, to protect your security and privacy
> and what not is completely moot if no one else actually looks at the code
> anyway. And let's be honest, the code quality of some of KDE's projects - I
> wouldn't touch them with a six-foot pole. The ones I would touch though,
> all have multiple people looking at the code and reviewing everything that
> goes in.
> 
> Let me be very clear - even if you're the best damn programmer on the
> planet, if *you* wrote the code, I do not trust *you* one inch to tell me
> that that code is correct. That verification needs to come from someone
> else, someone who does not have a conflict of interest in seeing that code
> get into production. This is nothing personal, this is confirmation bias on
> the author's part which leads to issues that even though they might be
> infrequent, usually have catastrophic impact.
> 
> And if "culture" trumps over engineering best practices, it follows that I
> should just stop using software produced by this entity because who knows
> what it's doing.

Mandatory code reviews are nice, if you have the manpower.

Look at our phabricator, look for example at the queue of reviews for 
KTextEditor.

The team is small and the code is complex and old (not rocket-science, it is a 
text editor,
but still...).

I and others tried to get more reviews done in the past, but actually I merged 
more
than once stuff that I reviewed but it did break the CI.

In most cases I fixed it myself afterwards or reverted again, but a few times
I needed to get ping'd by others that I was stupid, too.

In the current case discussed an error happend, it could have happend exactly 
the same
way if for example "me" would have reviewed it.

A lot of the changes which are at the moment in the queue are stuck because

a) lack of time to review the change
b) lack of time to ever be able to test the change

For any non-trivial behavior change (especially for features you not use 
yourself at all),
any meaningful review is a full-time job. e.g. in our company you would let 
some student
test the changed behavior some days. This is just not feasible for me, and yes,
for some of these changes, rather than abandoning them (and trashing precious 
work others did),
I will somewhen apply them with close to zero real testing.

Greetings
Christoph

-- 
- Dr.-Ing. Christoph Cullmann -
AbsInt Angewandte Informatik GmbH  Email: cullm...@absint.com
Science Park 1 Tel:   +49-681-38360-22
66123 Saarbrücken  Fax:   +49-681-38360-20
GERMANYWWW:   http://www.AbsInt.com

Geschäftsführung: Dr.-Ing. Christian Ferdinand
Eingetragen im Handelsregister des Amtsgerichts Saarbrücken, HRB 11234

D18296: Add support for passing cursor information via URL parameters when running kioclient exec.

[Christoph Cullmann]

cullmann added a reviewer: broulik.
cullmann added a comment.


  Perhaps Kai has some opinion.

REPOSITORY
  R126 KDE CLI Utilities

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

To: arrowd, #plasma, #ktexteditor, broulik
Cc: cullmann, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Differential] [Accepted] D4538: [KTextEditor] consistent conversion from/to cursor to/from coordinates

[Christoph Cullmann]

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


  Thanks for extending the test case + trying the KDevelop navigation!

REPOSITORY
  R39 KTextEditor

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

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

To: jsalatas, #frameworks, #plasma, #ktexteditor, cullmann
Cc: brauch, cullmann, plasma-devel, kwrite-devel, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4538: [KTextEditor] consistent conversion from/to cursor to/from coordinates

[Christoph Cullmann]

cullmann added a comment.


  > Currently if you try to convert the cursor to coordinates and then convert 
these coordinates back to a cursor again, it produces an invalid cursor (-1, 
-1) when the original cursor is located at the end of line
  >  Converting back to cursor coordinates that have been produced by 
cursorToCoordinate(view->cursorPosition()) and cursorPositionCoordinates() 
doesn't always produce the same results as the includeBorder option doesn't 
seem to be used consistently.
  
  Yeah, that seems strange
  
  > Although I cannot find a test case for this, it doen't seem to make any 
sense the line scrollPos(max, max.column(), calledExternally); in 
KateViewInternal::makeVisible function. Was it supposed to be that way (ie 
force when the last line is not empty and not force when it is), or is it just 
a typo? :\ I would appreciate any reasoning about this!
  
  Hmm, I doubt anybody can remember why that is that way ;=)
  
  Could your create some minimal unit test to ensure this stays consistent?

REPOSITORY
  R39 KTextEditor

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

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

To: jsalatas, #frameworks, #plasma, #ktexteditor
Cc: cullmann, plasma-devel, kwrite-devel, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

Fwd: Scrap baloo?

[Christoph Cullmann]

FYI, if you care, follow frameworks devel, guess double posting
only ends in pain.

Greetings
Christoph

- Weitergeleitete Mail -
Von: "cullmann" <cullm...@absint.com>
An: "kde-frameworks-devel" <kde-frameworks-de...@kde.org>
Gesendet: Mittwoch, 14. September 2016 23:29:22
Betreff: Scrap baloo?

Hi,

first, read that from my mail to the maintainer thread:



Hi,

after looking a bit more at the code, I think there are ATM a lot of things 
that need fixing:

1) 32-bit system: I see no fix, > 1GB of index and baloo + all baloo using 
applications fail

  see bugs like https://bugs.kde.org/show_bug.cgi?id=356114 here we have the 
5GB limit, which is now raised
  for 64-bit, but not for 32-bit

2) Larger filesystems: unfortunately one decided to ignore the upper 32-bit of 
the inodes

/**
 * Convert the QT_STATBUF into a 64 bit unique identifier for the file.
 * This identifier is combination of the device id and inode number.
 */
inline quint64 statBufToId(const QT_STATBUF& stBuf)
{
// We're loosing 32 bits of info, so this could potentially break
// on file systems with really large inode and device ids
return devIdAndInodeToId(static_cast(stBuf.st_dev),
 static_cast(stBuf.st_ino));
}

=> random breakage e.g. on my NFS drive here as the IDs clash and all 
invariants no longer hold.
(e.g. something can be a file but in addition a directory, )

3) No error handling of most lmdb faults (like already mentioned)

4) No error handling for any data corruption: e.g. many places will just 
endless loop or malloc, like
  DocumentUrlDB::get(quint64 docId) (we have bugs for that)

5) lmdb locking issues: crash one read-write process => all other things stall 
(or crash because of 3+4)

6) No resource management nor crash handling for the baloo_file_extractor which 
either OOMs you or corrupts the database on crash leading to 5)

CC'd Vishesh, perhaps I am wrong with that issues and misunderstand the code, 
unfortunately e.g. the database
structure is not that well documented, if I don't just not find the correct 
docs in the git.



Now executive summary, after a day more looking at the code.

1) 32-bit systems: never will be usable, thanks to lmdb, at least not with 
non-trivial index sizes

2) network file system homes: never will be usable, thanks to lmdb (ask its 
author: http://lmdb.tech/doc/ "Do not use LMDB databases on remote filesystems, 
even between processes on the same host. This breaks flock() on some OSes, 
possibly memory map sync, and certainly sync between programs on different 
hosts."

3) close to no error handling in the code => see the crash reports, I cleaned 
up a bit, but they are piling
  
https://bugs.kde.org/reports.cgi?product=frameworks-baloo=show_chart=CONFIRMED=ASSIGNED=REOPENED=UNCONFIRMED=RESOLVED=1

4) fundamental problems like: wrong data structure for index (32-bit inodes in 
21th century?) and close to zero docs what it does internally

Proposal:

Scrap baloo_file* and Co. and just reimplement the public API (modulo the 
settings for the then non-existing indexer daemon)
to use tracker.

Benefits:

1) Tracker is maintained: https://github.com/GNOME/tracker/graphs/contributors
2) We share the index with GNOME/* and save double indexing on "many" Linux 
systems which are not plain KDE Plasma Desktop based
3) We can delete 99% of the code (question is if we can remove the very buggy 
extractors from KFileMetaData, too, afterwards somewhen).

=> Opinions?

Greetings
Christoph

-- 
- Dr.-Ing. Christoph Cullmann -
AbsInt Angewandte Informatik GmbH  Email: cullm...@absint.com
Science Park 1 Tel:   +49-681-38360-22
66123 Saarbrücken  Fax:   +49-681-38360-20
GERMANYWWW:   http://www.AbsInt.com

Geschäftsführung: Dr.-Ing. Christian Ferdinand
Eingetragen im Handelsregister des Amtsgerichts Saarbrücken, HRB 11234
-- 
- Dr.-Ing. Christoph Cullmann -
AbsInt Angewandte Informatik GmbH  Email: cullm...@absint.com
Science Park 1 Tel:   +49-681-38360-22
66123 Saarbrücken  Fax:   +49-681-38360-20
GERMANYWWW:   http://www.AbsInt.com

Geschäftsführung: Dr.-Ing. Christian Ferdinand
Eingetragen im Handelsregister des Amtsgerichts Saarbrücken, HRB 11234

Re: Review Request 128892: Open baloo lmdb database read-only beside in baloo_file/baloo_file_extractor + balooctl (for some commands) + unit tests

[Christoph Cullmann]

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

(Updated Sept. 12, 2016, 12:39 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, Plasma, Boudhayan Gupta, and Vishesh Handa.


Changes
---

Submitted with commit 02047b524a176da447d8c96e15c7e2abae8339ae by Christoph 
Cullmann to branch master.


Repository: baloo


Description
---

At the moment, any application that uses baloo can corrupt the db.
Now, only the things that need to write to it open it with read-write.
This only works as long as the library exposes only read-only things like 
Query/...


Diffs
-

  src/engine/database.h 6ccb2a5 
  src/engine/database.cpp 6a433c7 
  src/file/extractor/app.cpp 0ca7276 
  src/lib/file.cpp cbbc912 
  src/lib/searchstore.cpp 060a4fd 
  src/lib/taglistjob.cpp 76ac8ff 
  src/qml/experimental/monitor.cpp 11c06ae 
  src/tools/balooctl/main.cpp 2a6b175 
  src/tools/balooctl/statuscommand.cpp 1a56c64 
  src/tools/balooshow/main.cpp f45f2e0 

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


Testing
---

Unit tests still work, balooctl seems to do something.


Thanks,

Christoph Cullmann

Re: Review Request 128892: Open baloo lmdb database read-only beside in baloo_file/baloo_file_extractor + balooctl (for some commands) + unit tests

[Christoph Cullmann]

> On Sept. 11, 2016, 9:35 p.m., Vishesh Handa wrote:
> > This is awesome. Ship it.

Lets hope it is awesome correct.


- Christoph


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


On Sept. 11, 2016, 7:19 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128892/
> ---
> 
> (Updated Sept. 11, 2016, 7:19 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma, Boudhayan Gupta, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> At the moment, any application that uses baloo can corrupt the db.
> Now, only the things that need to write to it open it with read-write.
> This only works as long as the library exposes only read-only things like 
> Query/...
> 
> 
> Diffs
> -
> 
>   src/engine/database.h 6ccb2a5 
>   src/engine/database.cpp 6a433c7 
>   src/file/extractor/app.cpp 0ca7276 
>   src/lib/file.cpp cbbc912 
>   src/lib/searchstore.cpp 060a4fd 
>   src/lib/taglistjob.cpp 76ac8ff 
>   src/qml/experimental/monitor.cpp 11c06ae 
>   src/tools/balooctl/main.cpp 2a6b175 
>   src/tools/balooctl/statuscommand.cpp 1a56c64 
>   src/tools/balooshow/main.cpp f45f2e0 
> 
> Diff: https://git.reviewboard.kde.org/r/128892/diff/
> 
> 
> Testing
> ---
> 
> Unit tests still work, balooctl seems to do something.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128892: Open baloo lmdb database read-only beside in baloo_file/baloo_file_extractor + balooctl (for some commands) + unit tests

[Christoph Cullmann]

> On Sept. 11, 2016, 7:19 p.m., Boudhayan Gupta wrote:
> > Can you add Vishesh and poke him if possible? This seems like a gigantic 
> > patch and I'm not sure I understand all of it right now.

Hi, I can poke him, sure.
Given there is no documentation what the things shall do, I am not 100% sure if 
that is perfect, myself.
On the other side: ATM all things open the database read-write, which imho is 
VERY bad.


- Christoph


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


On Sept. 11, 2016, 7:19 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128892/
> ---
> 
> (Updated Sept. 11, 2016, 7:19 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma, Boudhayan Gupta, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> At the moment, any application that uses baloo can corrupt the db.
> Now, only the things that need to write to it open it with read-write.
> This only works as long as the library exposes only read-only things like 
> Query/...
> 
> 
> Diffs
> -
> 
>   src/engine/database.h 6ccb2a5 
>   src/engine/database.cpp 6a433c7 
>   src/file/extractor/app.cpp 0ca7276 
>   src/lib/file.cpp cbbc912 
>   src/lib/searchstore.cpp 060a4fd 
>   src/lib/taglistjob.cpp 76ac8ff 
>   src/qml/experimental/monitor.cpp 11c06ae 
>   src/tools/balooctl/main.cpp 2a6b175 
>   src/tools/balooctl/statuscommand.cpp 1a56c64 
>   src/tools/balooshow/main.cpp f45f2e0 
> 
> Diff: https://git.reviewboard.kde.org/r/128892/diff/
> 
> 
> Testing
> ---
> 
> Unit tests still work, balooctl seems to do something.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Review Request 128892: Open baloo lmdb database read-only beside in baloo_file/baloo_file_extractor + balooctl (for some commands) + unit tests

[Christoph Cullmann]

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

Review request for KDE Frameworks, Plasma and Boudhayan Gupta.


Repository: baloo


Description
---

At the moment, any application that uses baloo can corrupt the db.
Now, only the things that need to write to it open it with read-write.
This only works as long as the library exposes only read-only things like 
Query/...


Diffs
-

  src/engine/database.h 6ccb2a5 
  src/engine/database.cpp 6a433c7 
  src/file/extractor/app.cpp 0ca7276 
  src/lib/file.cpp cbbc912 
  src/lib/searchstore.cpp 060a4fd 
  src/lib/taglistjob.cpp 76ac8ff 
  src/qml/experimental/monitor.cpp 11c06ae 
  src/tools/balooctl/main.cpp 2a6b175 
  src/tools/balooctl/statuscommand.cpp 1a56c64 
  src/tools/balooshow/main.cpp f45f2e0 

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


Testing
---

Unit tests still work, balooctl seems to do something.


Thanks,

Christoph Cullmann

Re: Review Request 128890: Make e.g. Baloo::Query thread safe.

[Christoph Cullmann]

> On Sept. 11, 2016, 6:32 p.m., Anthony Fieroni wrote:
> > src/engine/database.cpp, line 128
> > <https://git.reviewboard.kde.org/r/128890/diff/1/?file=476743#file476743line128>
> >
> > It's needed *m_env = 0;*
> > Or you can add method close to call it in all way.
> 
> Christoph Cullmann wrote:
> I don't understand the comment ;)
> 1) m_env != null means valid atm, therefore I can call here close
> 2) after this, the destructor is over, no need to null it, if you access 
> the database object after/during destruction, all is lost.
> 
> Anthony Fieroni wrote:
> Missing m_env = 0 after closing call. It can lead crash it future use.

Ah ;=) I see, I misread the line!
Thanks


- Christoph


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


On Sept. 11, 2016, 6:27 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128890/
> ---
> 
> (Updated Sept. 11, 2016, 6:27 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma and Boudhayan Gupta.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> lmdb itself is thread safe (e.g. you can use the same env in multiple 
> threads).
> Unfortunately, the Baloo::Database itself not, as open() might race against 
> other open calls (we have one unique db object in baloo).
> 
> => add non-recursive mutex (recursive mutex not needed, one just must avoid 
> to call isOpen() or path() inside open, that is done, else no unit test 
> works).
> 
> 
> Diffs
> -
> 
>   src/engine/database.h e3bb175 
>   src/engine/database.cpp ec7ae2e 
> 
> Diff: https://git.reviewboard.kde.org/r/128890/diff/
> 
> 
> Testing
> ---
> 
> Unit tests still work.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128890: Make e.g. Baloo::Query thread safe.

[Christoph Cullmann]

> On Sept. 11, 2016, 6:32 p.m., Anthony Fieroni wrote:
> > src/engine/database.cpp, line 128
> > <https://git.reviewboard.kde.org/r/128890/diff/1/?file=476743#file476743line128>
> >
> > It's needed *m_env = 0;*
> > Or you can add method close to call it in all way.

I don't understand the comment ;)
1) m_env != null means valid atm, therefore I can call here close
2) after this, the destructor is over, no need to null it, if you access the 
database object after/during destruction, all is lost.


- Christoph


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


On Sept. 11, 2016, 6:27 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128890/
> ---
> 
> (Updated Sept. 11, 2016, 6:27 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma and Boudhayan Gupta.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> lmdb itself is thread safe (e.g. you can use the same env in multiple 
> threads).
> Unfortunately, the Baloo::Database itself not, as open() might race against 
> other open calls (we have one unique db object in baloo).
> 
> => add non-recursive mutex (recursive mutex not needed, one just must avoid 
> to call isOpen() or path() inside open, that is done, else no unit test 
> works).
> 
> 
> Diffs
> -
> 
>   src/engine/database.h e3bb175 
>   src/engine/database.cpp ec7ae2e 
> 
> Diff: https://git.reviewboard.kde.org/r/128890/diff/
> 
> 
> Testing
> ---
> 
> Unit tests still work.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

Re: Review Request 128890: Make e.g. Baloo::Query thread safe.

[Christoph Cullmann]

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

(Updated Sept. 11, 2016, 9:27 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, Plasma and Boudhayan Gupta.


Changes
---

Submitted with commit e34da150d82a57cf417a59b8b632b2fecb32a6f7 by Christoph 
Cullmann to branch master.


Repository: baloo


Description
---

lmdb itself is thread safe (e.g. you can use the same env in multiple threads).
Unfortunately, the Baloo::Database itself not, as open() might race against 
other open calls (we have one unique db object in baloo).

=> add non-recursive mutex (recursive mutex not needed, one just must avoid to 
call isOpen() or path() inside open, that is done, else no unit test works).


Diffs
-

  src/engine/database.h e3bb175 
  src/engine/database.cpp ec7ae2e 

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


Testing
---

Unit tests still work.


Thanks,

Christoph Cullmann

Review Request 128890: Make e.g. Baloo::Query thread safe.

[Christoph Cullmann]

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

Review request for KDE Frameworks, Plasma and Boudhayan Gupta.


Repository: baloo


Description
---

lmdb itself is thread safe (e.g. you can use the same env in multiple threads).
Unfortunately, the Baloo::Database itself not, as open() might race against 
other open calls (we have one unique db object in baloo).

=> add non-recursive mutex (recursive mutex not needed, one just must avoid to 
call isOpen() or path() inside open, that is done, else no unit test works).


Diffs
-

  src/engine/database.h e3bb175 
  src/engine/database.cpp ec7ae2e 

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


Testing
---

Unit tests still work.


Thanks,

Christoph Cullmann

[Powerdevil] [Bug 350807] Desktop freezes when monitor switched off

[Christoph Cullmann]

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

Christoph Cullmann <cullm...@kde.org> changed:

   What|Removed |Added

 Resolution|--- |UPSTREAM
 CC||cullm...@kde.org
 Status|UNCONFIRMED |RESOLVED

--- Comment #4 from Christoph Cullmann <cullm...@kde.org> ---
Thats a Qt bug:

https://bugreports.qt.io/browse/QTBUG-42985

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Review Request 125528: Check for nullptr screen

[Christoph Cullmann]

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

Review request for Plasma and Marco Martin.


Bugs: 345173
http://bugs.kde.org/show_bug.cgi?id=345173


Repository: plasma-framework


Description
---

Check for nullptr screen


Diffs
-

  src/plasmaquick/dialog.cpp 8d4f508 

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


Testing
---

As Qt was stupid enough to change the API, we now need to check for nullptr.


Thanks,

Christoph Cullmann

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 125528: Check for nullptr screen

[Christoph Cullmann]

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

(Updated Oct. 5, 2015, 3 p.m.)


Status
--

This change has been marked as submitted.


Review request for Plasma and Marco Martin.


Changes
---

Submitted with commit 3f9109a2657a132879f29cc56471598a5f519b94 by Christoph 
Cullmann to branch master.


Bugs: 345173
http://bugs.kde.org/show_bug.cgi?id=345173


Repository: plasma-framework


Description
---

Check for nullptr screen


Diffs
-

  src/plasmaquick/dialog.cpp 8d4f508 

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


Testing
---

As Qt was stupid enough to change the API, we now need to check for nullptr.


Thanks,

Christoph Cullmann

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 125528: Check for nullptr screen

[Christoph Cullmann]

> On Oct. 5, 2015, 1:59 p.m., David Edmundson wrote:
> > src/plasmaquick/dialog.cpp, line 156
> > <https://git.reviewboard.kde.org/r/125528/diff/1/?file=409731#file409731line156>
> >
> > just QGuiApplication::screens() should be fine here.

Hmm, is then there not a regression that we look at screens not in the right 
"group"? At least I understood the docs that way that you don't get just all 
screens with virtualSiblings, but I am no expert.


- Christoph


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


On Oct. 5, 2015, 1:41 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125528/
> ---
> 
> (Updated Oct. 5, 2015, 1:41 p.m.)
> 
> 
> Review request for Plasma and Marco Martin.
> 
> 
> Bugs: 345173
> http://bugs.kde.org/show_bug.cgi?id=345173
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Check for nullptr screen
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/dialog.cpp 8d4f508 
> 
> Diff: https://git.reviewboard.kde.org/r/125528/diff/
> 
> 
> Testing
> ---
> 
> As Qt was stupid enough to change the API, we now need to check for nullptr.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 125528: Check for nullptr screen

[Christoph Cullmann]

> On Oct. 5, 2015, 1:59 p.m., David Edmundson wrote:
> > src/plasmaquick/dialog.cpp, line 156
> > <https://git.reviewboard.kde.org/r/125528/diff/1/?file=409731#file409731line156>
> >
> > just QGuiApplication::screens() should be fine here.
> 
> Christoph Cullmann wrote:
> Hmm, is then there not a regression that we look at screens not in the 
> right "group"? At least I understood the docs that way that you don't get 
> just all screens with virtualSiblings, but I am no expert.
> 
> David Edmundson wrote:
> theoretically there's a difference, yes:
> 
> (in X terms) An output's siblings will all be on the screen and you can 
> have multiple screens.
> You'd use the latter for pre-xinerama multihead. 
> 
> tbh I'd be super surprised if that still actually worked.
> 
> However, all we're doing is querying if a screen contains a point and 
> that should be the same.

Ok, I will just screens then.


- Christoph


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


On Oct. 5, 2015, 2:48 p.m., Christoph Cullmann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125528/
> ---
> 
> (Updated Oct. 5, 2015, 2:48 p.m.)
> 
> 
> Review request for Plasma and Marco Martin.
> 
> 
> Bugs: 345173
> http://bugs.kde.org/show_bug.cgi?id=345173
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Check for nullptr screen
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/dialog.cpp 8d4f508 
> 
> Diff: https://git.reviewboard.kde.org/r/125528/diff/
> 
> 
> Testing
> ---
> 
> As Qt was stupid enough to change the API, we now need to check for nullptr.
> 
> 
> Thanks,
> 
> Christoph Cullmann
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 125528: Check for nullptr screen

[Christoph Cullmann]

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

(Updated Oct. 5, 2015, 2:48 p.m.)


Review request for Plasma and Marco Martin.


Bugs: 345173
http://bugs.kde.org/show_bug.cgi?id=345173


Repository: plasma-framework


Description
---

Check for nullptr screen


Diffs (updated)
-

  src/plasmaquick/dialog.cpp 8d4f508 

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


Testing
---

As Qt was stupid enough to change the API, we now need to check for nullptr.


Thanks,

Christoph Cullmann

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel