Navigation context uses theme color.
Needs ReviewPublic

Authored by tristanp on Jun 29 2019, 8:46 PM.

Details

Reviewers
None
Group Reviewers
KDevelop
Summary

Previously the navigation context (tool displaying information about
code) was using fix white color theme. This was nice for people using
light themes but for dark themes it looked like a lighhouse in the night.

The solution proposed is to use colors from the common theme for
background but also for text and code displayed in the tool panel.
These colors are supposed to be contrasted enought and readable.

Modifications are limited to AbstractNavigationContext and sub classes.
AbstractNavigationContext hold a ColorizerTheme containing colors
named as before (m_typeHighlight, m_positionHighlight, ...), this theme
is retrieved from AbstractNavigationContext::theme() and every usage
of xxxHiglight(string) is replaced by theme().m_xxxHiglight(string).

Theme initialisation is proceeded into AbstractNavigationContext constructor
with a KColorScheme.

Note: This is my second patch in this projet, please let me know if I miss anything.

Diff Detail

Repository
R32 KDevelop
Branch
declaration_helper_color_theme
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13454
Build 13472: arc lint + arc unit
tristanp created this revision.Jun 29 2019, 8:46 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptJun 29 2019, 8:46 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
tristanp requested review of this revision.Jun 29 2019, 8:46 PM
tristanp edited the summary of this revision. (Show Details)Jun 29 2019, 8:47 PM
tristanp added a reviewer: KDevelop.
apol added a subscriber: apol.Jun 30 2019, 8:26 AM

Patch looks good to me. Would you be able to offer screenshots using Breeze?

kdevplatform/language/duchain/navigation/abstractnavigationcontext.h
77

Given they're public API, I would remove the m_ prefix.

tristanp updated this revision to Diff 60904.Jul 1 2019, 8:36 AM
  • Rename m_xxxHighlight to xxxHiglight as they are public.

Interesting approach, might work out, thanks for doing this :)

Not sure yet about all color mappings, that might need some more thinking, at least on a quick test across themes that resulted in quite some unreadable variants. Will play a bit during the upcoming WE.

One thing which might be nice to have added (later) is an automatic update on color scheme changes, which would be at least useful for the Code Browser tool view to not look out of place on a change (especially important on testing color schemes ;) ).

plugins/qmljs/duchain/navigation/declarationnavigationcontext.cpp
64

m_typeHighlight -> typeHighlight

Not sure yet about all color mappings, that might need some more thinking, at least on a quick test across themes that resulted in quite some unreadable variants. Will play a bit during the upcoming WE.

Ah, no, I confused myself with the used KColorScheme object of the running application not updating to colorscheme/palette changes of the UI. With a WIP prototype addition to update on UI colorscheme changes at runtime things look better or rather fine.

Will play some more and then propose the update mechanism as addition. Might be an idea to accept this patch already as is, for people not changing colorscheme at runtime ;)

kossebau added inline comments.Jul 8 2019, 1:13 PM
kdevplatform/language/duchain/navigation/abstractnavigationwidget.cpp
100

This comment though still applies, as this is about HTML injected from the documentation, e.g. about C++ classes.
Though admittedly this is currently partially broken at least for Qt documentation for me, will need testing for other documentation first.

tristanp added inline comments.Aug 2 2019, 6:57 PM
kdevplatform/language/duchain/navigation/abstractnavigationwidget.cpp
100

What palette is used with by default ? Does it follow the KColorScheme ?