Skip to content
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

Colorize entire split input header instead of just the channel name #3379

Merged
merged 28 commits into from Jan 22, 2022
Merged

Colorize entire split input header instead of just the channel name #3379

merged 28 commits into from Jan 22, 2022

Conversation

LosFarmosCTL
Copy link
Contributor

@LosFarmosCTL LosFarmosCTL commented Dec 2, 2021

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Having a lot of splits open in one tab currently results in a really frustrating experience when trying to navigate between them using keyboard shortcuts, since the only way of telling which split is currently active is the small colorized channel name in the split header.

This PR adds the option to draw the entire split header in a blue tint instead of just the channel name text. Adding a new setting for this is imo a good idea here, since there is no need for, what I am guessing is probably the majority of chatterino users, those who mostly have a single split in a tab to have their entire header colorized.

Screenshots

White

chatterino 2021-12-03 at 00 47 21

Light

chatterino 2021-12-03 at 00 47 02

Dark

chatterino 2021-12-03 at 00 45 37

Black

chatterino 2021-12-03 at 00 46 13

@leon-richardt
Copy link
Collaborator

I advocate for making this standard behavior and removing the option to switch to the old behavior.

@Mm2PL
Copy link
Collaborator

Mm2PL commented Dec 3, 2021

Should this really be an option? I'd rather just have the behavior changed.

@jupjohn
Copy link
Contributor

jupjohn commented Dec 3, 2021

Agree with Leon and mm2; I don't see a reason to keep the old behaviour around. I would like a single split to not have a colour though, maybe I'm special.

@LosFarmosCTL
Copy link
Contributor Author

I would like a single split to not have a colour though, maybe I'm special.

No, I feel the same way, its mainly why I made it a setting.

Maybe make it the default whenever there is more than one split currently opened in a tab?

@Felanbird
Copy link
Collaborator

I agree with whichever consensus does not like this change, but I also understand the reasoning for it.

@LosFarmosCTL
Copy link
Contributor Author

I changed the behaviour to be on by default but only show the colorized header if there is more than one split in the containing tab, retaining the previous behaviour for single splits.

@LosFarmosCTL LosFarmosCTL changed the title Add option to colorize the entire split input header instead of just the channel name Colorize entire split input header instead of just the channel name Dec 4, 2021
@zneix
Copy link
Collaborator

zneix commented Dec 30, 2021

I don't know if I am testing this wrong or misunderstanding the Pull Request, however - when there's only 1 split in the tab, it keeps the old behaviour; as in, the split header is still as it was before

@zneix
Copy link
Collaborator

zneix commented Dec 30, 2021

Found another regression:

dank_edge_case.mp4

@LosFarmosCTL
Copy link
Contributor Author

I don't know if I am testing this wrong or misunderstanding the Pull Request, however - when there's only 1 split in the tab, it keeps the old behaviour; as in, the split header is still as it was before

See my reply above, I agree with jammeh's take that coloring the entire header is unnecessary for a single split and more distracting than adding any additional value, so I added a check to retain the old behaviour for single splits.

Found another regression:

That one is definitely not intended though, I'll take a look at that tomorrow.

@pajlada
Copy link
Member

pajlada commented Jan 2, 2022

I agree that there should not be a setting for this, if we wanted this user-customizable imo we would support it through custom themes (which isn't off the table).

I don't think we should change the behaviour depending on the amount of splits in a tab - if it looks off with just one split I think it means that the behaviour might need tweaking.

With this information, I set off to tweak some things, and came up with this solution: Just change the focused background color of the split, but leave the color roughly the same as it was before this PR.

Multi split
image

Single split
image

Single split window not focused
image

Light mode single split focused
image

Light mode multi split with focus
image

PATCH
diff --git a/lib/qtkeychain b/lib/qtkeychain
--- a/lib/qtkeychain
+++ b/lib/qtkeychain
@@ -1 +1 @@
-Subproject commit de954627363b0b4bff9a2616f1a409b7e14d5df9
+Subproject commit de954627363b0b4bff9a2616f1a409b7e14d5df9-dirty
diff --git a/src/singletons/Theme.cpp b/src/singletons/Theme.cpp
index 98596a6e..bc506f9a 100644
--- a/src/singletons/Theme.cpp
+++ b/src/singletons/Theme.cpp
@@ -63,10 +63,10 @@ void Theme::actuallyUpdate(double hue, double multiplier)
     this->splits.header.border = getColor(0, sat, flat ? 1 : 0.85);
     this->splits.header.text = this->messages.textColors.regular;
     this->splits.header.focusedBackground =
-        getColor(0.58, 0.5, flat ? 0.9 : 0.85);
-    this->splits.header.focusedBorder = getColor(0.58, 0.5, flat ? 0.85 : 0.8);
-    this->splits.header.focusedText =
-        isLight ? QColor("#198CFF") : QColor("#84C1FF");
+        getColor(0, sat, isLight ? 0.95 : 0.79);
+    this->splits.header.focusedBorder = getColor(0, sat, isLight ? 0.90 : 0.78);
+    this->splits.header.focusedText = QColor::fromHsvF(
+        0.58388, isLight ? 1.0 : 0.482, isLight ? 0.6375 : 1.0);
 
     this->splits.input.background = getColor(0, sat, flat ? 0.95 : 0.95);
     this->splits.input.border = getColor(0, sat, flat ? 1 : 1);
diff --git a/src/widgets/splits/Split.cpp b/src/widgets/splits/Split.cpp
index a17442ec..937bfc11 100644
--- a/src/widgets/splits/Split.cpp
+++ b/src/widgets/splits/Split.cpp
@@ -1228,17 +1228,4 @@ void Split::drag()
     }
 }
 
-bool Split::isSingleSplit()
-{
-    if (auto container = dynamic_cast<SplitContainer *>(this->parentWidget()))
-    {
-        if (container->getSplits().size() == 1)
-        {
-            return true;
-        }
-    }
-
-    return false;
-}
-
 }  // namespace chatterino
diff --git a/src/widgets/splits/Split.hpp b/src/widgets/splits/Split.hpp
index fad68833..d0c0b6ba 100644
--- a/src/widgets/splits/Split.hpp
+++ b/src/widgets/splits/Split.hpp
@@ -75,8 +75,6 @@ public:
 
     void setContainer(SplitContainer *container);
 
-    bool isSingleSplit();
-
     static pajlada::Signals::Signal<Qt::KeyboardModifiers>
         modifierStatusChanged;
     static Qt::KeyboardModifiers modifierStatus;
diff --git a/src/widgets/splits/SplitHeader.cpp b/src/widgets/splits/SplitHeader.cpp
index f2603b2f..80edaf83 100644
--- a/src/widgets/splits/SplitHeader.cpp
+++ b/src/widgets/splits/SplitHeader.cpp
@@ -788,7 +788,7 @@ void SplitHeader::paintEvent(QPaintEvent *)
     QColor background = this->theme->splits.header.background;
     QColor border = this->theme->splits.header.border;
 
-    if (this->split_->hasFocus() && !this->split_->isSingleSplit())
+    if (this->split_->hasFocus())
     {
         background = this->theme->splits.header.focusedBackground;
         border = this->theme->splits.header.focusedBorder;
@@ -893,7 +893,7 @@ void SplitHeader::themeChangedEvent()
 {
     auto palette = QPalette();
 
-    if (this->split_->hasFocus() && this->split_->isSingleSplit())
+    if (this->split_->hasFocus())
     {
         palette.setColor(QPalette::WindowText,
                          this->theme->splits.header.focusedText);

@LosFarmosCTL
Copy link
Contributor Author

Had absolutely no time over the last few days, but I integrated pajladas patch now.

@zneix zneix self-requested a review January 14, 2022 18:49
Copy link
Collaborator

@zneix zneix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested all the changes, code looks good to me and I'm happy with the functionality it brings 👍 however if you want to make further changes later on, feel free to. Requested a review from pajlada, he can merge this once he reviews the PR.

@LosFarmosCTL
Copy link
Contributor Author

however if you want to make further changes later on, feel free to.

Dont have anything further to add to this PR either.

@zneix zneix requested a review from pajlada January 14, 2022 19:53
@pajlada pajlada merged commit b5b395f into Chatterino:master Jan 22, 2022
zneix added a commit to SevenTV/chatterino7 that referenced this pull request Jan 28, 2022
Now we're on commit 4e422b3; Changes from upstream we've pulled

+- Minor: Add search to emote popup. (Chatterino#3404, Chatterino#3527) // Chatterino#3527 got added, Chatterino#3404 was alredy merged in
+- Minor: Colorize the entire split header when focused. (Chatterino#3379)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants