D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Christoph Cullmann]

cullmann accepted this revision.
cullmann added a comment.


  I think this should go in now. I think the usage of the API is correct even 
for HiDPI.
  In KF6, we should just fold that interface into the main class.

REPOSITORY
  R39 KTextEditor

BRANCH
  addmarkinterfacev2

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

To: kossebau, #kate, #kdevelop, dhaumann, cullmann
Cc: cullmann, anthonyfieroni, dhaumann, kwrite-devel, kde-frameworks-devel, 
rrosch, LeGast00n, cblack, GB_2, domson, michaelh, ngraham, bruns, demsking, 
sars

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> dhaumann wrote in kateviewhelpers.cpp:1963
> This is true. But in this case a non-answer: Maybe QIcon::paint does it 
> correct as well. In other words, the code can very likely be improved, but 
> also ok as is.

@anthonyfieroni Why exactly do you think these calls should be kept, and how?

From what I understood the old code to do and tested before & after I had 
changed the code in the updated patch to now use QIcon::paint() instead of 
doing an own scaled QPixmap, the old logic used the devicePixelRatioF() calls 
as needed to match Qt's HiDPI support with internally bigger actual pixmaps. 
Whereas QIcon cares for that now, also in the case where the QIcon is created 
from a single pixmap set via `setMarkPixmap()` in the backward-compat case. So 
there is nothing to be done on our side anymore: we just estimate the "normal" 
size of the icon to be painted, and QIcon will do the actual painting matching 
whatever the HiDPI settings are, like it does in all other places QIcon is used.

REPOSITORY
  R39 KTextEditor

BRANCH
  addmarkinterfacev2

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

To: kossebau, #kate, #kdevelop, dhaumann
Cc: anthonyfieroni, dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, 
LeGast00n, cblack, GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, 
sars

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in kateviewhelpers.cpp:1963
> You should keep devicePixelRatioF calls

This is true. But in this case a non-answer: Maybe QIcon::paint does it correct 
as well. In other words, the code can very likely be improved, but also ok as 
is.

REPOSITORY
  R39 KTextEditor

BRANCH
  addmarkinterfacev2

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

To: kossebau, #kate, #kdevelop, dhaumann
Cc: anthonyfieroni, dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, 
LeGast00n, cblack, GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, 
sars

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> kossebau wrote in kateviewhelpers.cpp:1963
> Possibly `QIcon::paint()` might be also working here as wanted? Needs someone 
> with HiDPI to check if all things behave as wanted. The old code with all the 
> `devicePixelRatioF()` made me change not too much here.

You should keep devicePixelRatioF calls

REPOSITORY
  R39 KTextEditor

BRANCH
  addmarkinterfacev2

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

To: kossebau, #kate, #kdevelop, dhaumann
Cc: anthonyfieroni, dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, 
LeGast00n, cblack, GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, 
sars

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Dominik Haumann]

dhaumann accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R39 KTextEditor

BRANCH
  addmarkinterfacev2

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

To: kossebau, #kate, #kdevelop, dhaumann
Cc: anthonyfieroni, dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, 
LeGast00n, cblack, GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, 
sars

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Dominik Haumann]

dhaumann added a comment.


  Ok, then I am fine with this. Maybe add a KF6 task to the KF6 board?

INLINE COMMENTS

> kossebau wrote in katedocument.h:86
> See the warning in the API docs, nobody should rely on this private API, so 
> the BIC here is okay.
> The class is exported for the unit tests basically.
> Also is the header file not installed, so not really available outside.

Friedrich is correct. We have no issue here.

> katedocument.h:586
>  QHash m_marks;
> -QHash m_markPixmaps;
> +QHash m_markIcons; // QPixmap or QIcon, KF6: remove 
> QPixmap support
>  QHash m_markDescriptions;

Better turn " Remove pixmap support" into use QIcon only.

> katesearchbar.cpp:906
>  if (!m_highlightRanges.empty()) {
> -KTextEditor::MarkInterface *iface = 
> qobject_cast(m_view->document());
> +KTextEditor::MarkInterfaceV2 *iface = 
> qobject_cast(m_view->document());
>  if (iface) {

maybe use `auto` here.

> kossebau wrote in kateviewhelpers.cpp:1963
> Possibly `QIcon::paint()` might be also working here as wanted? Needs someone 
> with HiDPI to check if all things behave as wanted. The old code with all the 
> `devicePixelRatioF()` made me change not too much here.

I believe you can check yourself with proper environment variables, or?

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: anthonyfieroni, dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, 
LeGast00n, cblack, GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, 
sars

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in katedocument.h:86
> Why private class is exported, changing parent of an exported class is BIC.

See the warning in the API docs, nobody should rely on this private API, so the 
BIC here is okay.
The class is exported for the unit tests basically.
Also is the header file not installed, so not really available outside.

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: anthonyfieroni, dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, 
LeGast00n, cblack, GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, 
sars

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> katedocument.h:86
>  class KTEXTEDITOR_EXPORT KTextEditor::DocumentPrivate : public 
> KTextEditor::Document,
> -public 
> KTextEditor::MarkInterface,
> +public 
> KTextEditor::MarkInterfaceV2,
>  public 
> KTextEditor::ModificationInterface,

Why private class is exported, changing parent of an exported class is BIC.

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: anthonyfieroni, dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, 
LeGast00n, cblack, GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, 
sars

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D27533#615478 , @dhaumann wrote:
  
  > Still, I wonder whether we should postpone adding the MarkInterfaceV2 until 
the KF6 branch. Then we have it properly fixed in KF6.
  
  
  KF5 will stay around for a few more years though, if we compare to kdelibs3 & 
kdelibs4 life times. So IMHO it would be good/nice to already now allow more 
crisp symbols on the border (and in the default popup menu, which reuses the 
symbols passed, so right now the pixmap, again scaling to another size). Even 
more with SVG icons being the default now, so we can render as close as 
possible.
  
  The code I wrote did not feel like such a bummer, and the patch for Kate 
could perhaps just bump the required min version and spare the #if/#else. At 
time of porting it will be just an adaption of MarkInterfaceV2 back to 
MarkInterface on the client side. Merging MarkInterfaceV2 into MarkInterface in 
KTextEditor should also be simple work done in a few minutes, so no real 
technical debt added IMHO, compared to achieving a crisp symbol border.

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, 
GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, sars

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Dominik Haumann]

dhaumann added a comment.


  Definitely a +1 for using a QIcon here.
  
  Still, I wonder whether we should postpone adding the MarkInterfaceV2 until 
the KF6 branch. Then we have it properly fixed in KF6.
  
  Or do you think we also need this in KF5 days to properly support e.g. hidpi ?

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, 
GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, sars

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Friedrich W. H. Kossebau]

kossebau marked an inline comment as done.

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, GB_2, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 76127.
kossebau added a comment.


  Switch to QIcon::paint, leaving all vodoo with devicePixelRatioF to QIcon
  logic
  Also results in better handling of pixmaps when they would be oversampled.
  
  Tested with QT_SCALE_FACTOR

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27533?vs=76075=76127

BRANCH
  addmarkinterfacev2

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

AFFECTED FILES
  src/document/katedocument.cpp
  src/document/katedocument.h
  src/include/ktexteditor/markinterface.h
  src/search/katesearchbar.cpp
  src/view/kateviewhelpers.cpp

To: kossebau, #kate, #kdevelop
Cc: kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, GB_2, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> kateviewhelpers.cpp:1963
>  const int s = qMin(m_iconAreaWidth * 
> devicePixelRatioF(), h * devicePixelRatioF()) - 2;
> -px_mark = px_mark.scaled(s, s, 
> Qt::KeepAspectRatio, Qt::SmoothTransformation);
> +QPixmap px_mark = markIcon.pixmap(s);
> +px_mark.setDevicePixelRatio(devicePixelRatioF());

Possibly `QIcon::paint()` might be also working here as wanted? Needs someone 
with HiDPI to check if all things behave as wanted. The old code with all the 
`devicePixelRatioF()` made me change not too much here.

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, GB_2, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Friedrich W. H. Kossebau]

kossebau added a comment.


  See e.g. how bookmark mark symbols become crisp in bigger sizes (though they 
take advantage currently in being mostly regular lines and thus partially cope 
with pixelscaling):
  F8116357: Screenshot_20200221_003940.png 


REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, GB_2, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Friedrich W. H. Kossebau]

kossebau added a comment.


  https://invent.kde.org/kde/kate/merge_requests/68 would be the respective 
patch for Kate.

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, GB_2, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D27533: [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks

[Friedrich W. H. Kossebau]

kossebau created this revision.
kossebau added reviewers: Kate, KDevelop.
Herald added projects: Kate, Frameworks.
Herald added subscribers: kde-frameworks-devel, kwrite-devel.
kossebau requested review of this revision.

REVISION SUMMARY
  Symbols used for marks can be used in different sizes, e.g. depending of the
  line height, to which the icon border adapts, or for the context menu on
  actions to toggle those marks. Being limited to set a single pixmap as
  symbol for a mark results can result in badly scaled symbols being
  displayed.
  Switching to QIcon as dynamic pixmap provider for markers improves this.
  
  For backward compatibility QIcon & QPixmaps are converted into each other in
  case APIs are used mixed.
  
  Currently this is WIP as KDevelop as user of the mark interfaces partially
  relies on providing a single pixmap only, and while taking the pixmaps from
  QIcons does some QIcon::Mode-based rendering (for breakpoints, marking
  disabled, reached, pending or normal ones) or color-tinting (marking line
  removal/addition in patch display with positive & negative colors) before
  passing the pixmaps on. I am not yet sure whether to
  a) turning all those processings into explisit separate dedicatd icons
  
(my favourite)
  
  b) adding some additional rendering flags to marks
  is the best approach.
  
  If going for a), this patch would be final as is.

REPOSITORY
  R39 KTextEditor

BRANCH
  addmarkinterfacev2

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

AFFECTED FILES
  src/document/katedocument.cpp
  src/document/katedocument.h
  src/include/ktexteditor/markinterface.h
  src/search/katesearchbar.cpp
  src/view/kateviewhelpers.cpp

To: kossebau, #kate, #kdevelop
Cc: kwrite-devel, kde-frameworks-devel, rrosch, LeGast00n, cblack, GB_2, 
domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann