Navigation context uses theme color.
Needs ReviewPublic
Actions

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

Details

Reviewers

None

Group Reviewers

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

Harbormaster completed remote builds in B13414: Diff 60847.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.

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.

Rename m_xxxHighlight to xxxHiglight as they are public.

Harbormaster completed remote builds in B13454: Diff 60904.Jul 1 2019, 8:36 AM

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

In D22158#491427, @kossebau wrote:

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 ?

Would it be possible and interesting to reuse code highlighting for the code quote ?

hey! can you also show a screenshot of PHP or Qt documentation showing? these can contain arbitrary HTML and sometimes even contain colored text that expects to be shown on bright backgrounds which would break with this patch. At least that was the case years ago when I worked on this the last time.

due to that, we decided to not use a dark color palette, as it could potentially lead to unreadable text in the documentation popup

if that has changed, then I'm all for it!

e.g. look at https://doc.qt.io/archives/qt-4.8/qcolor.html#predefined-colors

Revision Contents
Changeset List

			Path	Packages
M			kdevplatform/language/duchain/navigation/abstractdeclarationnavigationcontext.cpp (54 lines)
M			kdevplatform/language/duchain/navigation/abstractincludenavigationcontext.cpp (6 lines)
M			kdevplatform/language/duchain/navigation/abstractnavigationcontext.h (43 lines)
M			kdevplatform/language/duchain/navigation/abstractnavigationcontext.cpp (53 lines)
M			kdevplatform/language/duchain/navigation/abstractnavigationwidget.cpp (8 lines)
M			kdevplatform/language/duchain/navigation/problemnavigationcontext.cpp (4 lines)
M			kdevplatform/language/duchain/navigation/usesnavigationcontext.cpp (2 lines)
M			plugins/clang/duchain/macronavigationcontext.cpp (2 lines)
M			plugins/qmljs/duchain/navigation/declarationnavigationcontext.cpp (2 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	60847	64e6ae6		Jun 29 2019, 8:46 PM	★	★
Diff 2	60904	64e6ae6	- Rename m_xxxHighlight to xxxHiglight as they are public.	Jul 1 2019, 8:36 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
498e0650fcac	57e896ab1d77	fd9523fa0862	tristan	Rename m_xxxHighlight to xxxHiglight as they are public.	Jul 1 2019, 9:32 AM
fd9523fa0862	7b2dc28e94eb	64e6ae6e717d	tristan	Navigation context uses theme color. (Show More…)	Jun 15 2019, 9:28 PM