D20445: signalplotter: Add highdpi support

[Alexander Volkov]

volkov added a comment.


  Beams look ugly when their width is >= 4 pixels.
  The idea is to draw them with the same width regardless of DPI.
  Another option is to not set Qt::FlatCap for thick pens, but I prefer this 
one.

REPOSITORY
  R111 KSysguard Library

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

To: volkov
Cc: davidedmundson, zzag, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20445: signalplotter: Add highdpi support

[David Edmundson]

davidedmundson added a comment.


  > To avoid artifacts when painting beams.
  
  It might avoid it, but it doesn't explain why it makes sense.
  setWidth should be in logical sizes. It gets affected by the view transform.

REPOSITORY
  R111 KSysguard Library

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

To: volkov
Cc: davidedmundson, zzag, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20445: signalplotter: Add highdpi support

[Alexander Volkov]

volkov added inline comments.

INLINE COMMENTS

> zzag wrote in ksignalplotter.cpp:897-899
> Could you please explain why we have to do this?

To avoid artifacts when painting beams.

REPOSITORY
  R111 KSysguard Library

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

To: volkov
Cc: zzag, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20445: signalplotter: Add highdpi support

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> ksignalplotter.cpp:897-899
> +pen.setWidthF(1.0 / dpr);
>  else
> +pen.setWidthF(2.0 / dpr);

Could you please explain why we have to do this?

REPOSITORY
  R111 KSysguard Library

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

To: volkov
Cc: zzag, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20445: signalplotter: Add highdpi support

[Alexander Volkov]

volkov updated this revision to Diff 56102.
volkov added a comment.


  use QPen::setWidthF() instead of QPen::setWidth()

REPOSITORY
  R111 KSysguard Library

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20445?vs=56079&id=56102

BRANCH
  master

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

AFFECTED FILES
  signalplotter/ksignalplotter.cpp

To: volkov
Cc: zzag, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20445: signalplotter: Add highdpi support

[Alexander Volkov]

volkov updated this revision to Diff 56079.
volkov added a comment.


  reduced beam width

REPOSITORY
  R111 KSysguard Library

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20445?vs=55922&id=56079

BRANCH
  master

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

AFFECTED FILES
  signalplotter/ksignalplotter.cpp

To: volkov
Cc: zzag, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20445: signalplotter: Add highdpi support

[Alexander Volkov]

volkov added a comment.


  Hi, thanks for the review.
  I know about the issues, I've uploaded the patch to avoid losing it :)

REPOSITORY
  R111 KSysguard Library

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

To: volkov
Cc: zzag, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20445: signalplotter: Add highdpi support

[Vlad Zagorodniy]

zzag added a comment.


  Yep, those issues are still present. :(
  F6764644: foo.png 

REPOSITORY
  R111 KSysguard Library

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

To: volkov
Cc: zzag, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20445: signalplotter: Add highdpi support

[Vlad Zagorodniy]

zzag added a comment.


  Oh, nice! FWIW, I made a patch to add HighDPI support some time ago as well. 
I haven't uploaded it for review because there were issues with beams not being 
smooth.

INLINE COMMENTS

> ksignalplotter.cpp:681-682
> +const auto dpr = mScrollableImage.devicePixelRatio();
> +QPointF point = boundingBox.topRight() - QPointF(mScrollOffset - 2, 
> 0);
> +QRectF sourceRect(0, 0, (mScrollOffset - 1) * dpr, 
> boundingBox.height() * dpr);
>  #ifdef USE_QIMAGE

Can be const.

> ksignalplotter.cpp:691
>  if(widthOfSecondHalf > 0) {
> +const auto dpr = mScrollableImage.devicePixelRatio();
> +QRectF sourceRect(mScrollableImage.width() - (widthOfSecondHalf + 1) 
> * dpr, 0, widthOfSecondHalf * dpr, boundingBox.height() * dpr);

Don't abuse `auto`.

REPOSITORY
  R111 KSysguard Library

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

To: volkov
Cc: zzag, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20445: signalplotter: Add highdpi support

[Alexander Volkov]

volkov created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
volkov requested review of this revision.

REPOSITORY
  R111 KSysguard Library

BRANCH
  master

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

AFFECTED FILES
  signalplotter/ksignalplotter.cpp

To: volkov
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart