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

[Christoph Cullmann]

cullmann abandoned this revision.

REPOSITORY
  R39 KTextEditor

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

To: cullmann, #kdevelop, #ktexteditor, #kate, mwolff, intelfx
Cc: kossebau, cullmann, kde-frameworks-devel, brauch, dhaumann, mwolff, 
kwrite-devel, hase, michaelh, ngraham, bruns, demsking, sars

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

[Christoph Cullmann]

cullmann commandeered this revision.
cullmann added a reviewer: intelfx.
cullmann added a comment.


  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.

REPOSITORY
  R39 KTextEditor

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

To: cullmann, #kdevelop, #ktexteditor, #kate, mwolff, intelfx
Cc: kossebau, cullmann, kde-frameworks-devel, brauch, dhaumann, mwolff, 
kwrite-devel, hase, michaelh, ngraham, bruns, demsking, sars

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

[Christoph Cullmann]

cullmann added a comment.


  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 
.

REPOSITORY
  R39 KTextEditor

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

To: intelfx, #kdevelop, #ktexteditor, #kate, mwolff
Cc: kossebau, cullmann, kde-frameworks-devel, brauch, dhaumann, mwolff, 
kwrite-devel, hase, michaelh, ngraham, bruns, demsking, sars

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

[Ivan Shapovalov]

intelfx added a comment.


  In D5802#349761 , @dhaumann wrote:
  
  > - 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. It 
could make sense to actually exert some effort to support it properly.

REPOSITORY
  R39 KTextEditor

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

To: intelfx, #kdevelop, #ktexteditor, #kate, mwolff
Cc: kossebau, cullmann, kde-frameworks-devel, brauch, dhaumann, mwolff, 
kwrite-devel, michaelh, ngraham, bruns, demsking, sars

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

[Dominik Haumann]

dhaumann added a comment.


  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? ;)

REPOSITORY
  R39 KTextEditor

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

To: intelfx, #kdevelop, #ktexteditor, #kate, mwolff
Cc: kossebau, cullmann, kde-frameworks-devel, brauch, dhaumann, mwolff, 
kwrite-devel, michaelh, ngraham, bruns, demsking, sars

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

[Friedrich W. H. Kossebau]

kossebau added a comment.


  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?

REPOSITORY
  R39 KTextEditor

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

To: intelfx, #kdevelop, #ktexteditor, #kate, mwolff
Cc: kossebau, cullmann, kde-frameworks-devel, brauch, dhaumann, mwolff, 
kwrite-devel, michaelh, ngraham, bruns, demsking, sars

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

[Ivan Shapovalov]

intelfx added a comment.


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

REPOSITORY
  R39 KTextEditor

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

To: intelfx, #kdevelop, #ktexteditor, #kate, mwolff
Cc: cullmann, kde-frameworks-devel, brauch, dhaumann, mwolff, kwrite-devel, 
michaelh, kevinapavew, ngraham, bruns, demsking, sars

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

[Christoph Cullmann]

cullmann added a comment.


  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.

REPOSITORY
  R39 KTextEditor

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

To: intelfx, #kdevelop, #ktexteditor, #kate, mwolff
Cc: cullmann, kde-frameworks-devel, brauch, dhaumann, mwolff, kwrite-devel, 
michaelh, kevinapavew, ngraham, bruns, demsking, sars

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

[Ivan Shapovalov]

intelfx added a comment.


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

REPOSITORY
  R39 KTextEditor

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

To: intelfx, #kdevelop, #ktexteditor, #kate, mwolff
Cc: kde-frameworks-devel, brauch, dhaumann, mwolff, kwrite-devel, michaelh, 
kevinapavew, ngraham, bruns, demsking, cullmann, sars

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

[Sven Brauch]

brauch added a comment.
Herald added a project: Kate.
Herald edited subscribers, added: kde-frameworks-devel; removed: Frameworks.


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

REPOSITORY
  R39 KTextEditor

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

To: intelfx, #kdevelop, #ktexteditor, #kate, mwolff
Cc: kde-frameworks-devel, brauch, dhaumann, mwolff, kwrite-devel, michaelh, 
kevinapavew, ngraham, bruns, demsking, cullmann, sars, #frameworks

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

[Milian Wolff]

mwolff requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R39 KTextEditor

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

To: intelfx, #kdevelop, #ktexteditor, #kate, mwolff
Cc: dhaumann, mwolff, kwrite-devel, #frameworks

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

[Milian Wolff]

mwolff added a comment.


  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.

REPOSITORY
  R39 KTextEditor

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

To: intelfx, #kdevelop, #ktexteditor, #kate
Cc: dhaumann, mwolff, kwrite-devel, #frameworks

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

[Dominik Haumann]

dhaumann added a comment.


  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?

REPOSITORY
  R39 KTextEditor

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

To: intelfx, #kdevelop, #ktexteditor, #kate
Cc: dhaumann, mwolff, kwrite-devel, #frameworks

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

[Ivan Shapovalov]

intelfx added a comment.


  In https://phabricator.kde.org/D5802#109416, @mwolff wrote:
  
  > 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?

REPOSITORY
  R39 KTextEditor

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

To: intelfx, #kdevelop, #ktexteditor, #kate
Cc: mwolff, kwrite-devel, #frameworks

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

[Ivan Shapovalov]

intelfx added a comment.


  In https://phabricator.kde.org/D5802#109416, @mwolff wrote:
  
  > 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).
  
  F3752512: KDevelop-Vim (dark)-Before.png 

  
  F3752511: KDevelop-Vim (dark)-After.png 
  
  F3752510: KDevelop-Printing-Before.png 
  
  F3752509: KDevelop-Printing-After.png 
  
  F3752508: KDevelop-Normal-Before.png 
  
  F3752507: KDevelop-Normal-After.png 
  
  F3752506: KDevelop-KDE-Before.png 
  
  F3752505: KDevelop-KDE-After.png 
  
  F3752504: KDevelop-Breeze Dark-Before.png 

  
  F3752503: KDevelop-Breeze Dark-After.png 

  
  So, basically, Normal and Printing are broken because they are explicitly 
designed for 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).
  
  F3752527: katesyntaxhighlightingrc 
  
  F3752526: kateschemarc 

REPOSITORY
  R39 KTextEditor

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

To: intelfx, #kdevelop, #ktexteditor, #kate
Cc: mwolff, kwrite-devel, #frameworks

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

[Milian Wolff]

mwolff added a comment.


  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?

REPOSITORY
  R39 KTextEditor

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

To: intelfx, #kdevelop, #ktexteditor, #kate
Cc: mwolff, kwrite-devel, #frameworks

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

[Ivan Shapovalov]

intelfx added a comment.


  In https://phabricator.kde.org/D5802#109403, @mwolff wrote:
  
  > 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?

REPOSITORY
  R39 KTextEditor

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

To: intelfx, #kdevelop, #ktexteditor, #kate
Cc: mwolff, kwrite-devel, #frameworks

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

[Milian Wolff]

mwolff added a comment.


  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.

REPOSITORY
  R39 KTextEditor

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

To: intelfx, #kdevelop, #ktexteditor, #kate
Cc: mwolff, kwrite-devel, #frameworks

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

[Ivan Shapovalov]

intelfx edited the summary of this revision.

REPOSITORY
  R39 KTextEditor

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

To: intelfx, #kdevelop, #ktexteditor, #kate
Cc: kwrite-devel, #frameworks

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

[Ivan Shapovalov]

intelfx edited the summary of this revision.

REPOSITORY
  R39 KTextEditor

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

To: intelfx, #kdevelop, #ktexteditor, #kate
Cc: kwrite-devel, #frameworks