ViewPrivate, KateSearchBar, KateVi::MatchHighlighter: use selection foreground for search highlights
AbandonedPublic

Authored by cullmann on May 10 2017, 10:49 PM.

Details

Reviewers
mwolff
intelfx
Group Reviewers
KDevelop
KTextEditor
Kate
Summary

This is not an actual pull-request but rather an RFC (request for comments) from
the KTextEditor, Kate and KDevelop developers, because I failed to solicit any
feedback on the kdevelop-devel@ and kwrite-devel@ mailing lists.

(http://kde.6490.n7.nabble.com/Properly-porting-Solarized-to-Kate-KDevelop-need-to-change-behavior-of-search-result-highlighting-td1655274.html)

So, I'm trying to make an exact port of Solarized to Kate/KDevelop and it looks
like Solarized is not compatible with Kate's search result highlighting behavior:
it requires search/replace highlight regions to use selection foreground and not
the normal one. You can find the details in the linked archive post.

The interesting detail is that it seems like kdevplatform's context browser plugin
hints at behavior sought by me (but implements it incorrectly, thus incidentally
remaining consistent with all other highlighting-related features in KTE/Kate).

(https://github.com/KDE/kdevplatform/blob/master/plugins/contextbrowser/contextbrowser.cpp#L657)

However, just making that change will make Kate/KDevelop incompatible with all
existing color schemes. Hence, how do I proceed?

Diff Detail

Repository
R39 KTextEditor
Branch
selection-foreground-for-search-highlights
Lint
No Linters Available
Unit
No Unit Test Coverage
intelfx created this revision.May 10 2017, 10:49 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 10 2017, 10:49 PM
Restricted Application added subscribers: Frameworks, kwrite-devel. · View Herald Transcript
intelfx edited the summary of this revision. (Show Details)May 10 2017, 11:00 PM
intelfx edited the summary of this revision. (Show Details)
mwolff added a subscriber: mwolff.May 14 2017, 10:22 AM

Hey there,

sorry for the long delay. In general this sounds like a good idea. But what do you mean by "However, just making that change will make Kate/KDevelop incompatible with all existing color schemes"? Can you show some before/after screenshots for an existing color scheme? I mean for your solarized theme it looks fine. I think using this feature everywhere would be a good idea. So if needed, the other color schemes could/should be adapted - but I really first would need to see the effect in action.

Hey there,

sorry for the long delay. In general this sounds like a good idea. But what do you mean by "However, just making that change will make Kate/KDevelop incompatible with all existing color schemes"? Can you show some before/after screenshots for an existing color scheme? I mean for your solarized theme it looks fine.

I mean that just like Solarized relies on using selection foreground for highlights (otherwise it is illegible), some other color scheme may rely on using normal foreground for its highlights. Hence just making this change unconditionally may break someone's existing color scheme.

Even if we adjust bundled color schemes for the new behavior, we cannot adjust everyone's custom color schemes. Provided that we care for them, of course.

I think using this feature everywhere would be a good idea. So if needed, the other color schemes could/should be adapted - but I really first would need to see the effect in action.

Do you want screenshots of Solarized or any other color scheme?

For solarized you showed the screenshots in your original mail. I'm more concerned about backwards compatibility with other schemes. I.e. yes - we do care about the status quo. Can you give an example for a color scheme where this would break stuff? Then I can also apply the patch locally and try it out myself and maybe come up with a concrete idea to fix this all.

Also, can you then share your solarized theme?

intelfx added a comment.EditedMay 14 2017, 11:10 AM

For solarized you showed the screenshots in your original mail. I'm more concerned about backwards compatibility with other schemes. I.e. yes - we do care about the status quo. Can you give an example for a color scheme where this would break stuff? Then I can also apply the patch locally and try it out myself and maybe come up with a concrete idea to fix this all.

OK — take a look at the attached screenshots (I hope Phabricator will preserve their names).

So, basically, Normal and Printing are broken because they are explicitly designed for the old behavior. Breeze-Dark and Vim-dark do not care because they have selection foreground == normal foreground.

Also, can you then share your solarized theme?

Please find attached in form of rc files (append them to your kateschemarc/katesyntaxhighlightingrc, I've stripped all other color schemes).

Then I can also apply the patch locally and try it out myself and maybe come up with a concrete idea to fix this all.

So, do you have any ideas?

I am not yet convinced this is a good idea: We changed the foreground color of highlighted text just recently: https://git.reviewboard.kde.org/r/127554/diff/2/

The idea is to have just one color for search results that need to match. This currently is the foreground color of normal text. We would need to really make sure all color schemes still work properly.

As I understand, this is the case?

It's not the case, printing and normal are broken now e.g.

Couldn't this be fixed by checking what color has the higher contrast - selection or normal? Then pick the one that has the higher contrast.

mwolff requested changes to this revision.Nov 19 2017, 10:18 PM
This revision now requires changes to proceed.Nov 19 2017, 10:18 PM
brauch added a subscriber: brauch.Aug 14 2018, 9:21 PM

Can't we simply update our shipped schemas, and expect users with custom schemas to fix them?

Restricted Application added a project: Kate. · View Herald TranscriptAug 14 2018, 9:21 PM
Restricted Application edited subscribers, added: kde-frameworks-devel; removed: Frameworks. · View Herald Transcript

@mwolff @dhaumann @brauch I'm sorry for my silence — I stopped using KDE/KDevelop some time ago and did not track this changeset properly. Could you please reiterate what was the conclusion and what were @mwolff's requested changes?

The idea is to have just one color for search results that need to match. This currently is the foreground color of normal text. We would need to really make sure all color schemes still work properly.

Sorry, I did not understand this. Could you please rephrase?

Couldn't this be fixed by checking what color has the higher contrast - selection or normal? Then pick the one that has the higher contrast.

IMO this is too specific logic to hard-code. What if there is a schema explicitly designed for low contrast?

Can't we simply update our shipped schemas, and expect users with custom schemas to fix them?

I believe many would oppose :)

I think we don't want to change that now.
We perhaps will port all things to the KSyntaxHighlighting theming and then we will alter this behavior anyways.

@cullmann So, how do you propose to port Solarized to Kate/KDevelop then?

No insight into the topic, but seeing this in my review request list (KDevelop):
Given KTextEditor was ported to KSyntaxHighlighting now, what can be said about this patch? Does it still apply? What foreground color options are there now for search highlights?

Or should this patch/RFC be abandoned officially, given discussion has petered out with no results/goals?

As I see we have the following situation

  • the current implementation does work reasonably well
  • just because we cannot match a theme 100% does not imply that we have to adapt our implementation (there will always be a color in theme xy that does not properly exist in KTextEditor)
  • KTextEditor does not use color themes from KSyntaxHighlighting yet, so the transition is not complete

Given no action in a long time, this indeed looks like a 'reject'. Still, maybe we should reevaluate? Volunterrs? ;)

intelfx added a comment.EditedOct 28 2018, 6:59 PM
  • just because we cannot match a theme 100% does not imply that we have to adapt our implementation (there will always be a color in theme xy that does not properly exist in KTextEditor)

Solarized is not just a random color scheme. It's a pretty popular one, and rather unique in that it was actually designed around a pretty cool idea rather than a bunch of colors thrown together that happened to look kind of nice.

It could make sense to actually exert some effort to support it properly.

For: It could make sense to actually exert some effort to support it properly.

There seems to be no interest to spend effort on by anybody.
Therefore I would say we shall close this and D5803.

cullmann commandeered this revision.Dec 8 2018, 5:33 PM
cullmann added a reviewer: intelfx.

As nobody will be working on this, I abandon this now.
If somebody steps up to do the work, feel free to reopen it again.

cullmann abandoned this revision.Dec 8 2018, 5:33 PM