Bug 309553 - [Minimap] Problem with alternative colour scheme and widget misalignment
Summary: [Minimap] Problem with alternative colour scheme and widget misalignment
Status: RESOLVED FIXED
Alias: None
Product: kate
Classification: Applications
Component: application (show other bugs)
Version: Git
Platform: unspecified Linux
: NOR normal
Target Milestone: ---
Assignee: KWrite Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-04 20:31 UTC by Emmanuel Lepage Vallée
Modified: 2015-08-19 07:28 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
Synced progressbar prototype (12.31 KB, application/octet-stream)
2012-11-05 22:08 UTC, Emmanuel Lepage Vallée
Details
Antialiased rounded rects (2.11 KB, patch)
2012-11-06 05:28 UTC, Emmanuel Lepage Vallée
Details
A new patch with designer inputs (5.64 KB, patch)
2015-07-30 13:43 UTC, Emmanuel Lepage Vallée
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Emmanuel Lepage Vallée 2012-11-04 20:31:43 UTC
Hi,
Take a look at this screenshot:
http://ompldr.org/vZzU4aQ/blender148.png

#1:
There is 2 Kate instances, one running with dark colours and the other with the default scheme (using a second user account). The one with the default colours look just fine. However, the one using darker one use the same background??? It was not the same yesterday.

I think the palette().base().color() should be used, so the same colour as the katepart background and a line to mark the area used by the scrollbar and the area used by the katepart.

#2:
In the darker Kate, I disabled scrollbar arrows to highlight the second issue. As you can see on the top of the scrollar, there is a 5 pixels misalignment in the top and bottom due to the Oxygen theme inner border. To fix this, calling the style on the painter with some options will draw the correct border. Just use the painter or CSS to make sure it does not paint the left border in LTR mode and right in RTL mode.

#Mockup:
Here is a "fixed" version made in KolourPaint

http://ompldr.org/vZzU5NA

I did:
* Remove the 5 pixel misalignment
* I kept the arrow disabled, but it look just as good with them enabled, in fact, even better
* Extended the inner border around palette()->base() area to also cover the scrollbar
* Removed the inner border on the left and replaced it with a line with 10% opacity. It have to be white if the scheme is dark and black when it is bright. The code to detect that use in almost every apps, you can find one implementation here https://projects.kde.org/projects/playground/network/sflphone-kde/repository/revisions/october2012_ui/entry/src/klib/tip.cpp#L97
* I avoided to make the 10% line full size, as it look to merge with the blue border and it is not as nice as having a few pixel truncated at the top and bottom. 
* Made the "visible area rectangle" rounded instead of sharp. Not necessary, but seem to make it look better. It also make it "alien" to the rest of the code and other rectangles. This is good because it make it easier to the brain to detect it is not part of the background. Just as buttons are a little 3D above the background in dialogs. It make it easy to know they are a separated entity. 

Also, just as a comment, I think it would be nice if the map could be resized to 250px, even if it take a little more CPU. With high resolution display (like my MacBook Pro Retina), it is entirely impossible to see where functions start and end. It is supposed to be the point of this feature in term of usability. This is just a comment, otherwise, I love this new feature, it is awesome.

Reproducible: Always

Steps to Reproduce:
1. Use a darker scheme with get from 4 november.
Comment 1 Kåre Särs 2012-11-05 13:53:05 UTC
Thanks for the report.

This feature is just a week old and we get bug reports. It looks like it is a 
wanted feature :)

#1: (Colors) Should be fixed in trunk. The shield colors are now taken from the 
palette and the lightness is adjusted according to the foreground/background 
lightness.

#2: (Alignment) The text component has a border that the custom scrollbar does 
not have in MiniMap mode. This must be investigated/fixed eventually


#Mockup:
-Rounded corners is a nice idea
-Extending the border of the text area around the scrollbar would be nice, but 
is not worth the effort at the moment. It would mean that the scrollbar and the 
text area would have to synchronize the the painting... not a trivial task... 
Patches are welcome ;)

#Scaling up will stretch the pixmap and that might not look so good, but the 
option is there already. I just need to change the limit. There is still a 
hard limit of ~100 characters per line for performance reasons

/Kåre
Comment 2 Emmanuel Lepage Vallée 2012-11-05 22:08:02 UTC
Created attachment 75036 [details]
Synced progressbar prototype

Synced progressbar prototype
Comment 3 Emmanuel Lepage Vallée 2012-11-05 22:09:44 UTC
>Extending the border of the text area around the scrollbar would be nice, but is not worth the effort at 
>the moment. It would mean that the scrollbar and the text area would have to synchronize the the 
> painting... not a trivial task... Patches are welcome ;)

Why not! The code is attached as a prototype to this message. It is actually quite simple to sync them, a little harder to display the border correctly.

The sync trick is to add an event filter to the viewPort() widget looking for focus and hover in and out. This will give you the infos you need to modify the paintEvent to draw the correct state.

For the border painting part, I honestly had no idea, so I took code from qframe.cpp and adapted it to run on a QScrollBar. It miss the animation part, it is not very fluid compared to a real border fade animation. I have no idea how to fix that one. What do you think?

I did not implement the bacground color and the hide border on the left because it does not apply to your mini map implementation. I also committed to draw the scrollbar content to keep the code simple.
Comment 4 Emmanuel Lepage Vallée 2012-11-06 05:28:02 UTC
Created attachment 75048 [details]
Antialiased rounded rects

This simple patch enable antialiasing for the rounder rect and add a 1px margin on the left and right of the rect. The margin make it feel like the rect is part of the map instead of cutting it in 2. It also make the rounded part separated from the map border, preventing the perception both side are not symmetric. 

I would also like to point that you have 2 rects drawing on top of each other. I am not so sure what's the point of this. It does create quite a few glitch http://ompldr.org/vZzV0eQ . If there is a purpose, please tell me. The Y mismatch is present on all files, but greater in longer files. You use KMag to spot them.
Comment 5 Kåre Särs 2012-11-08 22:18:54 UTC
Thanks,

I have added tried to fix the alignment issue. (still a bit off at the bottom)

2 rects: if you have a really large file the visible area becomes much smaler than the slider -> draw the slider and the visible area separately.

Y mismatch: It is not always easy to get it right ;) I have tried now with master and I can't notice it.

The bigest problem with the synced focus is that I would need to also modify the text view and that is not something I'm too eager on doing right now ;)

Regards,
  Kåre
Comment 6 Emmanuel Lepage Vallée 2012-11-09 02:49:46 UTC
Hi,

>The bigest problem with the synced focus is that I would need to also modify the text view and that is not something I'm too eager on doing right now ;)

-Last time I talk about that, but as I shown in the prototype attached to this bug, it does not require any modifications as focus tracking is a core QtGui feature. The only modification is hiding the right border, there is few way to do this without too much code. But there's still the bug in my prototype where the fadein animation is not disaplay. As long as that's not solved, it's a no go.

-Alignment (katepart vs scrollbar) seem to be fixed. There is an extra pixel on the bottom, but it is mostly invisible. 

-Y align for the 2 rect is still a little bit off (only visible when using KMag to track it) but only my 1 px. What about disabling the second rect when the whole view fit in a single page?

-What about supporting color option for the background color and rect color (keep your as default)? If I was willing to implement it, could I just go ahead and commit it? I don't want to step on your feet while you are working on this.

-Thanks for merging the anti-aliasing part of the patch I posted, but what about the other change, is there a rational why it is bad or something? Oxygen have a 2px margin for scrollbar for the same rationals as I talked earlier.

Still, thanks for working on this. Having these "modern" features in Kate is really, really cool! I complain a lot on details, I am aware of it, I just want this to be as awesome as it can get. Enough to be willing to put my code where my mouth is (and I don't do that very often).
Comment 7 Kåre Särs 2012-11-09 10:16:09 UTC
> --- Comment #6 from Emmanuel Lepage Vallée <elv1313@gmail.com> ---
> Hi,
> 
> >The bigest problem with the synced focus is that I would need to also
> >modify the text view and that is not something I'm too eager on doing
> >right now ;)
> -Last time I talk about that, but as I shown in the prototype attached to
> this bug, it does not require any modifications as focus tracking is a core
> QtGui feature. The only modification is hiding the right border, there is
> few way to do this without too much code. But there's still the bug in my
> prototype where the fadein animation is not disaplay. As long as that's not
> solved, it's a no go.

The thing I'm not too eager about is the hiding of the right part of the focus 
for the KateView.


> 
> -Alignment (katepart vs scrollbar) seem to be fixed. There is an extra pixel
> on the bottom, but it is mostly invisible.
> 
> -Y align for the 2 rect is still a little bit off (only visible when using
> KMag to track it) but only my 1 px. What about disabling the second rect
> when the whole view fit in a single page?
The ting is how do I detect this reliably...,  Hmm maybe if the slider is more 
than 2 pixels bigger.. I'll check it out.

> 
> -What about supporting color option for the background color and rect color
> (keep your as default)? If I was willing to implement it, could I just go
> ahead and commit it? I don't want to step on your feet while you are
> working on this.
I would like to avoid adding that option. Kate already contains quite a bunch 
of different colors to configure. The colors are now taken from the theme and 
light adjusted for the used foreground/background colors. If it seems broken 
for many we might re-think it.

> 
> -Thanks for merging the anti-aliasing part of the patch I posted, but what
> about the other change, is there a rational why it is bad or something?
> Oxygen have a 2px margin for scrollbar for the same rationals as I talked
> earlier.
I re-read your bug report after I had committed the anti-aliasing. I will 
apply the margin when I get to it :) I think 5 pixels rounding might be a bit 
too much... 

> 
> Still, thanks for working on this. Having these "modern" features in Kate is
> really, really cool! I complain a lot on details, I am aware of it, I just
> want this to be as awesome as it can get. Enough to be willing to put my
> code where my mouth is (and I don't do that very often).
I think it is great that so many want this feature and are willing to give 
feedback and even patches :)


/Kåre
Comment 8 Emmanuel Lepage Vallée 2012-11-10 22:42:28 UTC
The problems I have with the colours used for background and foreground is inconsistency. Example:

http://ompldr.org/vZzgzcA

In this example, I have to point a few things:
*The slider background is black for C++ and red for Lua. Why? Because the color order for those 2 languages is not the same. So alert is first in Lua and last in C++. So, in the end, the foreground is totally arbitrary. But I love that red! It may be an accident, but I like it. Too bad only lua have "alert" as first colour...

*The "unseen area" background is also different. It create a few problem of it's own:

------For C++, it ain't so bad. I guess you based your math on that one. For Lua however, it is about as bright as the "content" so the brightness differential index is so low that the brain have to focus to see the lines.  At least mine do.

------In both care the brightness index of the scrollbar is much higher then the code area. This is problematic for many reasons. (it is the opposite in brighter palette, and in that case, it ain't so bad)

------------First that is high contrast is usually bad for the eye because switching focus between bright and dark area on a screen too often cause the pigment that allow us to see in the dark to be "burned" to turn out vision into day light one. But when focusing back on the dark area, our eye rush to create those things again. Doing that too often will hurt the eye night vision permanently. http://en.wikipedia.org/wiki/Night_vision#Biological_night_vision

------------Second, more related to usability, is the reason why professional multimedia application use dark colours palette. It allow to eye to quickly focus on content, like video editing focus on the video itself. On pure bright application, eye is overwhelmed by light, so it focus on the application rather than part of it. By creating out of control brightness differential between the text canvas and scrollbar, you accidentally create such a focus hub. 

While the problem is not present with Oxygen default color palette and while looking at C++, the current color model does not scale beyond those default settings. My opinion would be to use the palette().base().color() or Kate current text background colour as background and using use a brightness index averaging the document brightness index on the "mini-lines" to make sure the scrollbar is not distracting. And for the slider background, just adding a config option, the current one is just as random as it can get. It also void the palette in both dark and bright mode (in birght/light mode, bitghter is "on top" while darker is "below", see the normal scrollbar for an example of that.). But then, it may be a good idea to have a little bigger pool of opinions.
Comment 9 Emmanuel Lepage Vallée 2015-07-30 13:43:08 UTC
Created attachment 93802 [details]
A new patch with designer inputs

New patch
Comment 10 Kåre Särs 2015-07-31 21:32:29 UTC
Looks good, I'm not 100% sure about the transparency level of the slider handle and the fading of "invisible" area tho ;)

You can commit from my point of view. We can tweak the levels later if needed.
Comment 11 Kåre Särs 2015-08-19 07:28:55 UTC
Git commit 9c0df13721838c2e395f94c413529d170ef96163 by Kåre Särs, on behalf of Emmanuel Lepage Vallee.
Committed on 19/08/2015 at 07:26.
Pushed by sars into branch 'master'.

minimap: Attempt to improve the look and feel

This patch attempt to make the minimap look better.

* The design has been reviewed during Akademy
* It now feel "native" in the canvas for both Breeze and Fusion
  styles. Oxygen has some hardcoded extra borders, this is a
  known limitation of this patch.
* Improve support for darker themes. The previous implementation
  failed to properly display the visible area
* Partially degrade the visible section view when the handle is
  at its minimum height. This could be improved later on.

M  +54   -34   src/view/kateviewhelpers.cpp

http://commits.kde.org/ktexteditor/9c0df13721838c2e395f94c413529d170ef96163