-
Notifications
You must be signed in to change notification settings - Fork 865
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Small improvements to the “Show parent folder name in tab title” option #7930
Small improvements to the “Show parent folder name in tab title” option #7930
Conversation
I think visually distinguishing file name from path could be useful, but it will be probably tricky to find a color which works on active/inactive tabs for both light and dark themes. (e.g the FlatLaf light screenshot is a bit hard to read for me at least) The test output window reduces the contrast for less important stack frames for example (#6695 has screenshots). This is how it computes the color: netbeans/java/junit.ui/src/org/netbeans/modules/junit/ui/api/JUnitCallstackFrameNode.java Lines 63 to 73 in 7d89336
It does also use the colors from the actual component as input instead of using a hardcoded value. |
btw If you can't access the actual tab component, this might help to figure out what the key is - if it has one: UIManager.getLookAndFeelDefaults().entrySet().stream()
.filter( t -> t.getValue() instanceof Color)
.sorted((o1, o2) -> o1.getKey().toString().compareTo(o2.getKey().toString()))
.forEach(t -> System.out.println(t.getKey()+ ": " + t.getValue())); |
1591db6
to
7136c44
Compare
@mbien thanks for the example. Made the changes and updated the screenshots. |
what you see here is the html renderer noticing that the contrast is too low and trying to fix it to set the color to the complementary white or black. This is the opposite of what the intention is since it is now highlighting the folder instead of the file. As previously mentioned the tricky part is to make it work on different background / foregrounds - its used on different components and component states. I reduced the fade value to 0.7f and computed it for the tabs and tabs-switcher separately, since both have different colors. Tested on all supported themes and it looks ok I think. The "same background color for same projects" mode is still not working very well, esp on dark themes, but making both decorators work well together won't be easy. here the commit c4ea291, feel free to squash it with your PR |
7136c44
to
8fa82e7
Compare
@mbien, thank you so much for your help! |
@troizet not a problem with me. But you can add co-authors to commits by appending this to the commit message:
|
8fa82e7
to
913d29c
Compare
@mbien, thank you! |
@troizet while looking through other code, I noticed NB has a boolean property for dark themes, we can use that for having a different blend value based on dark/light themes: diff --git a/platform/core.multitabs/src/org/netbeans/core/multitabs/impl/FolderNameTabDecorator.java b/platform/core.multitabs/src/org/netbeans/core/multitabs/impl/FolderNameTabDecorator.java
index 421cdc9..f1a64fd 100644
--- a/platform/core.multitabs/src/org/netbeans/core/multitabs/impl/FolderNameTabDecorator.java
+++ b/platform/core.multitabs/src/org/netbeans/core/multitabs/impl/FolderNameTabDecorator.java
@@ -124,10 +124,14 @@
}
private String fadeColor(Color f, Color b) {
- float a = 0.7f;
+ float a = isDarkLaF() ? 0.7f : 0.6f;
return String.format("#%02x%02x%02x", //NOI18N
(int)(b.getRed() + a * (f.getRed() - b.getRed())),
(int)(b.getGreen() + a * (f.getGreen() - b.getGreen())),
(int)(b.getBlue() + a * (f.getBlue() - b.getBlue())));
}
+
+ private static boolean isDarkLaF() {
+ return UIManager.getBoolean("nb.dark.theme"); //NOI18N
+ }
} I could force push into this PR if you want since I have it locally already anyway. |
Yes, please do. Thank you! |
913d29c
to
2a6a4f0
Compare
I do like this, however, the main reason I am reluctant to approve this is because this might make the "colored tabs" situation worse (#8029). we probably should wait with this until we have an idea how to fix the colored tabs? cc @neilcsmith-net @eirikbakke edit: or we could simply not apply the contrast reduction to the parent folder if colored tabs are enabled? |
You can usually make text blend in with any background color by using transparency (alpha channel) instead of reduced saturation. For example, to make 50% "grey" text, you would use black with 50% transparency on light themes, and white with 50% transparency on dark themes. I.e. use (If that works, we don't have to wait for improvements in the background color before making improvements to the text color.) |
@eirikbakke I don't think the html renderer supports alpha. It seems to ignore the fourth value even if I set it to 0. (the underlying issue is that the contrast is already so low, that blending it will make it hard to read on some components, esp when features like colored tabs come in and swap out the background without adjusting the foreground color - but using alpha would be still nice) |
@mbien Alpha blending might be useful enough to make HtmlRenderer.findColor aware of it. It saves clients from having to query and blend background colors themselves. |
2a6a4f0
to
013a28c
Compare
String folderName = folder.getNameExt() + pathSeparator; | ||
String defaultText = tab.getText(); | ||
|
||
return merge( folderName, defaultText ); | ||
String folderName = folder.getNameExt() + File.separator; | ||
// don't use faded colors in tabs when colored tabs are active | ||
// since it is difficult to get right with so many colors and shades involved | ||
// the switcher uses a line as marker instead of bg change and can continue using fade colors | ||
if (!(isTab && settings.isSameProjectSameColor())) { | ||
folderName = "<font color=\"" + fadeColor + "\">" + folderName + "</font>"; //NOI18N | ||
} | ||
return merge(folderName, tab.getText()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the path decoration will now disable itself on tabs if colored tabs are active, the switcher will keep using the faded color since its colored tabs impl is different
an attempt to fix some contrast problems mentioned in #7930 (comment) is pending at #8175
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
013a28c
to
892234d
Compare
- parent folder name is displayed in opened documents list - parent folder name is displayed in a different color from the file name Co-authored-by: Michael Bien <[email protected]>
rebasing on top of #8175 |
892234d
to
ad776db
Compare
As I was asked for review, I'm not going to request changes (at least at this point), but I think the approach to colouring is wrong and over complicated. We should be looking to move all colouring code like this out of components. This should be a UI key. Something like a tab secondary foreground colour, that can use FlatLaf support for mixing and contrast, and the defaults on other LaFs. And would also ideally be used for the problematic versioning info. |
@neilcsmith-net Your main objection is with the fadeColor method that has a hard-coded alpha value in it, right? Is there any reason the alpha value cannot come from one of the parameterized Color instances instead? Here is a more general "blend" function that takes alpha values into account, in the same way as when you paint one semitransparent color on top of a solid (or itself semitransparent) background color:
If useful, I could add it to org.openide.util.Utilities. I've been using it for several years. |
No, my main objection is that the faded colour is not coming from a UI key (ideally a shared one), probably with a suitable fallback to an existing key. That could be looked at for NB 26. I have a whole bunch of similar blending code functions I could contribute. But so does FlatLaf, and I firmly believe this belongs in the LaF configuration. LaFs without overridable accent colours, etc. can have a fixed value / use an existing key anyway. Think separation of concerns like HTML and CSS. |
@neilcsmith-net I agree, it's always nice to have these constants separated out. Though often it can be useful to experiment for a bit to figure out what the constants should be and what color differences we need to express, before factoring them out as constants and making them part of an official API contract. (Though "API contract" probably means a less strong promise of forward compatibility when it comes to cosmetic UI properties, than when it comes to classes and method signatures.) |
agreed. see #8175 (comment), as mentioned we will also need a separate value for dark/light themes. (and it likely won't be just a color or blend factor, it will probably have to be a computed color based on usage conditions, similar to the selection foreground color) |
are there concerns outside of the hardcoded blend function + factors? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested the latest version on Light and Dark themes, with and without "Show parent folder name in tab title" enabled, and also testing with the "Sort opened documents list by project" in some cases. Nothing breaks or is unreadable in this version. Thanks for the PR!
Small improvements to the “Show parent folder name in tab title” option:
Before:
After:
After dark theme: