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

Add start_separator #2087

Merged
merged 2 commits into from
Nov 14, 2024
Merged

Add start_separator #2087

merged 2 commits into from
Nov 14, 2024

Conversation

lorber13
Copy link
Contributor

Linked to issue #2066

@bim9262
Copy link
Collaborator

bim9262 commented Oct 18, 2024

@siddarth2810 raises a good point on the issue, the color for the start separator should match the first segment of the first block.

Something like...
diff --git a/src/protocol.rs b/src/protocol.rs
index df6007ff0..5ee8877b1 100644
--- a/src/protocol.rs
+++ b/src/protocol.rs
@@ -25,11 +25,17 @@ where
     let mut prev_last_bg = Color::None;
     let mut rendered_blocks = vec![];
 
-    // The right most block should never be alternated
-    let mut alt = blocks
+    let blocks_with_segments: Vec<RenderedBlock> = blocks
         .iter()
         .map(|x| x.borrow())
-        .filter(|x| !x.segments.is_empty() && !x.merge_with_next)
+        .filter(|x| !x.segments.is_empty())
+        .cloned()
+        .collect();
+
+    // The right most block should never be alternated
+    let mut alt = blocks_with_segments
+        .iter()
+        .filter(|x| !x.merge_with_next)
         .count()
         % 2
         == 0;
@@ -42,17 +48,21 @@ where
         rendered_blocks.push(I3BarBlock {
             full_text: start_separator.clone(),
             background: Color::None,
-            color: prev_last_bg,
+            // Peek at the first segment of the first first block to see what color it is
+            color: blocks_with_segments.first().map_or(Color::None, |widget| {
+                widget.segments.first().map_or(Color::None, |segment| {
+                    if alt {
+                        segment.background + config.theme.alternating_tint_bg
+                    } else {
+                        segment.background
+                    }
+                })
+            }),
             ..Default::default()
         });
     }
 
-    for widgets in blocks
-        .iter()
-        .map(|x| x.borrow())
-        .filter(|x| !x.segments.is_empty())
-        .cloned()
-    {
+    for widgets in blocks_with_segments {
         let RenderedBlock {
             mut segments,
             merge_with_next,

@bim9262
Copy link
Collaborator

bim9262 commented Oct 18, 2024

Both your code and what I wrote yesterday don't replace the first separator, but just add another in front, but perhaps it would be better to replace it with the specified separator like

<start separator, if set, else separator><block 1><separator><block 2>...<end separator, if set>

Perhaps
diff --git a/src/protocol.rs b/src/protocol.rs
index df6007ff0..72e774701 100644
--- a/src/protocol.rs
+++ b/src/protocol.rs
@@ -38,20 +38,12 @@ where
 
     let mut prev_merge_with_next = false;
 
-    if let Separator::Custom(start_separator) = &config.theme.start_separator {
-        rendered_blocks.push(I3BarBlock {
-            full_text: start_separator.clone(),
-            background: Color::None,
-            color: prev_last_bg,
-            ..Default::default()
-        });
-    }
-
-    for widgets in blocks
+    for (i, widgets) in blocks
         .iter()
         .map(|x| x.borrow())
         .filter(|x| !x.segments.is_empty())
         .cloned()
+        .enumerate()
     {
         let RenderedBlock {
             mut segments,
@@ -73,7 +65,12 @@ where
             alt = !alt;
         }
 
-        if let Separator::Custom(separator) = &config.theme.separator {
+        let separator = match &config.theme.start_separator {
+            Separator::Custom(_) if i == 0 => &config.theme.start_separator,
+            _ => &config.theme.separator,
+        };
+
+        if let Separator::Custom(separator) = separator {
             if !prev_merge_with_next {
                 // The first widget's BG is used to get the FG color for the current separator
                 let sep_fg = if config.theme.separator_fg == Color::Auto {

@siddarth2810
Copy link

siddarth2810 commented Oct 19, 2024

wow so if i am not wrong block_with_segments part was creating an extra theme separator at the start, so you used i=0 to check if its first block and only use start separator for that. So cool

If its alright, I can take this up :)

@lorber13
Copy link
Contributor Author

lorber13 commented Oct 26, 2024

Both your code and what I wrote yesterday don't replace the first separator, but just add another in front, but perhaps it would be better to replace it with the specified separator like

<start separator, if set, else separator><block 1><separator><block 2>...<end separator, if set>
Perhaps

diff --git a/src/protocol.rs b/src/protocol.rs
index df6007ff0..72e774701 100644
--- a/src/protocol.rs
+++ b/src/protocol.rs
@@ -38,20 +38,12 @@ where
 
     let mut prev_merge_with_next = false;
 
-    if let Separator::Custom(start_separator) = &config.theme.start_separator {
-        rendered_blocks.push(I3BarBlock {
-            full_text: start_separator.clone(),
-            background: Color::None,
-            color: prev_last_bg,
-            ..Default::default()
-        });
-    }
-
-    for widgets in blocks
+    for (i, widgets) in blocks
         .iter()
         .map(|x| x.borrow())
         .filter(|x| !x.segments.is_empty())
         .cloned()
+        .enumerate()
     {
         let RenderedBlock {
             mut segments,
@@ -73,7 +65,12 @@ where
             alt = !alt;
         }
 
-        if let Separator::Custom(separator) = &config.theme.separator {
+        let separator = match &config.theme.start_separator {
+            Separator::Custom(_) if i == 0 => &config.theme.start_separator,
+            _ => &config.theme.separator,
+        };
+
+        if let Separator::Custom(separator) = separator {
             if !prev_merge_with_next {
                 // The first widget's BG is used to get the FG color for the current separator
                 let sep_fg = if config.theme.separator_fg == Color::Auto {

Why do we need a start separator if the default separator is already present before the first block? I'm starting to have doubts about its usefulness.
EDIT: after reading again the issue description, it seems that the behavior we want is the ability to replace the first separator. Sorry, I misunderstood.

Copy link
Collaborator

@MaxVerevkin MaxVerevkin left a comment

Choose a reason for hiding this comment

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

Does not currently work as expected, please apply @bim9262's suggestion.

@MaxVerevkin MaxVerevkin linked an issue Nov 10, 2024 that may be closed by this pull request
@MaxVerevkin MaxVerevkin merged commit ae2f92f into greshake:master Nov 14, 2024
13 checks passed
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.

Replace starter separator?
4 participants