Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks
ClosedPublic

Authored by kossebau on Feb 20 2020, 11:27 PM.

Details

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.

Diff Detail

Repository
R39 KTextEditor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau created this revision.Feb 20 2020, 11:27 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptFeb 20 2020, 11:27 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
kossebau requested review of this revision.Feb 20 2020, 11:27 PM

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):

kossebau added inline comments.Feb 20 2020, 11:44 PM
src/view/kateviewhelpers.cpp
1960–1961

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.

kossebau updated this revision to Diff 76127.Feb 21 2020, 8:43 PM

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

kossebau marked an inline comment as done.Feb 21 2020, 8:43 PM

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 ?

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.

anthonyfieroni added inline comments.
src/document/katedocument.h
86

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

kossebau added inline comments.Feb 22 2020, 8:53 AM
src/document/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.

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

src/document/katedocument.h
86

Friedrich is correct. We have no issue here.

586

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

src/search/katesearchbar.cpp
906

maybe use auto here.

src/view/kateviewhelpers.cpp
1960–1961

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

dhaumann accepted this revision.Feb 22 2020, 10:41 AM
This revision is now accepted and ready to land.Feb 22 2020, 10:41 AM
src/view/kateviewhelpers.cpp
1960–1961

You should keep devicePixelRatioF calls

dhaumann added inline comments.Feb 23 2020, 12:36 PM
src/view/kateviewhelpers.cpp
1960–1961

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.

kossebau added inline comments.Feb 24 2020, 7:45 AM
src/view/kateviewhelpers.cpp
1960–1961

@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.

cullmann accepted this revision.Feb 28 2020, 5:01 PM
cullmann added a subscriber: cullmann.

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.

kossebau retitled this revision from [WIP] Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks to [Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks.Thu, Mar 5, 3:01 PM
kossebau updated this revision to Diff 77041.Thu, Mar 5, 3:04 PM

Do proper static_cast into QMetaType enum

Having thought some more about the KDevelop cases, their pixmap preprocessing really should be replaced by proper custom icons, so the currently proposed MarkInterfaceV2 should be fine also for KDevelop.
Thus going to land this post 5.68 tagging then as is. Thanks everyone for review :)

kossebau retitled this revision from [Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks to Add MarkInterfaceV2, to s/QPixmap/QIcon/g for symbols of marks.Thu, Mar 5, 3:07 PM
kossebau edited the summary of this revision. (Show Details)

Thanks for working on this.
Badly scaled stuff is a real eyesore...

This revision was automatically updated to reflect the committed changes.