Add Visual Indication to Identify Modified Files in Tabline

[smolck]

This PR makes gnvim show the + symbol for a modified file in the tabline:

See #51.

I implemented this based off of vhakulinen's info in #51, but feedback/suggestions are much appreciated.

[badosu]

Just my two cents, I prefer a visual indication without text (something that is not straightfoward on TUI but is on GUI), like a different color in a certain place of the tab, a light fade, a yellow fill on the leftmost rectangle of the tab, etc..

That said, it's only a matter of taste and this is already better than what we have (and a direct reference to neovim's original behavior), so I'm fine with it.

[smolck]

In theory, that could be done with css, right? I do agree that it would look better to have something fancier, and I have no problem working on implementing it for this PR, but it may take some work. I don’t know much about how gnvim implements the css styling atm, so any help would be appreciated.

Also, a visual indication like what you stated could set apart gnvim from other neovim frontends, since afaik other nvim GUIs don’t have that (just something simple e.g. the vscode-like dot in Oni), so I’m all for it (as long as it remains obvious that the tab has a modified buffer).

[badosu]

I fiddled around with it, this is a style that could work (you'd have to change to use a variable: if not modified use the line above, if modified the one below in tab:checked and tab:hover). First is not current tab, second is hover and third is current tab if all were modified.

diff --git a/src/ui/tabline.rs b/src/ui/tabline.rs
index 689de72..075348c 100644
--- a/src/ui/tabline.rs
+++ b/src/ui/tabline.rs
@@ -162,10 +162,12 @@ impl Tabline {
                 background-color: #{normal_bg};
                 border: none;
                 box-shadow: inset -2px -70px 10px -70px rgba(0,0,0,0.75);
+                box-shadow: inset 73px 0px 0px -70px rgba(244, 226, 66, 0.4);
             }}
             tab:checked {{
                 border: none;
                 box-shadow: inset 73px 0px 0px -70px #{selected_fg};
+                box-shadow: inset 73px 0px 0px -70px rgba(244, 226, 66, 1);
             }}
             tab:checked, tab:checked > label {{
                 color: #{selected_fg};
@@ -173,6 +175,7 @@ impl Tabline {
             }}
             tab:hover {{
                 box-shadow: inset 73px 0px 0px -70px #{selected_fg};
+                box-shadow: inset 73px 0px 0px -70px rgba(244, 226, 66, 0.80);
             }}
             ",
             font_wild = self.font.as_wild_css(FontUnit::Point),

[badosu]

The biggest downside of this approach is that colors might not be the best way forward, if we get colors from the colorscheme then we have inconsistent UI (modified indicator color changes between gnvim colorschemes), if not we may have issues (e.g. a weird yellow background colorscheme, but that's a very edge case, I hope).

Given above, and considering that the '+' solution is what vim already does and users expect, I think it is a safer approach in terms of complexity and UX.

[smolck]

I do like the idea, and it looks good, but at first the difference between the second and third tabs was a bit hard for me to see. I think varying colors (i.e. one color for active/inactive that varies in shade like you showed, but maybe a slightly/completely different color for modified) may be easier to see and understand, but the problems you said are good points to consider. Not to mention that this whole idea would add more code to determine if the tab’s buffer is modified and to change the css accordingly. I can work on it, but it could always be done in a later PR if not this one, assuming we want to go forward with the idea. (I definitely like your idea more than the ‘+’ icon in the tabline, since it fits the GUI better, so I’ll tinker with it.)

[badosu]

Yeah, good points.

I think we can start with this iteration and revisit it later, if we deem necessary, what do you think?

[smolck]

Yeah, I think that’s a good idea. That way we can go ahead and get this feature in gnvim, since it is nice to have, and beautify it in the future.

We could also, after this PR is over, open up an issue for beautifying this feature’s look, so that we have a place to work on it and an issue to reference in a future PR. Think we should do that?

(Also, thanks @badosu for all the help on polishing/fixing/changing this PR and my other one, and on the corresponding issues. Helps a lot, and I really appreciate it.)

[smolck]

I abstracted the tab_label logic, and also handled some possible errors. Let me know what you think about the way I handled the errors. They needed to be handled because gnvim would crash when I went back and forth between modified tabs (not exactly sure how to reproduce, but I think it was sending the requests from the tabpage too quickly/frequently to nvim), which is no good. After handling the errors by just setting modified to false in get_tab_label if an error occurred, gnvim didn't crash when I tested it.

[vhakulinen]

Some changes to keep things more streamlined:

diff --git a/src/ui/tabline.rs b/src/ui/tabline.rs
index 49a8f3d..7cbfdf4 100644
--- a/src/ui/tabline.rs
+++ b/src/ui/tabline.rs
@@ -6,8 +6,8 @@ use glib;
 use gtk;
 use gtk::prelude::*;
 use neovim_lib::{
-    neovim_api::{NeovimApi, Tabpage},
-    Neovim,
+    neovim_api::{Buffer, NeovimApi, Tabpage},
+    Neovim, Value,
 };
 use pango;
 
@@ -68,37 +68,46 @@ impl Tabline {
         self.notebook.clone().upcast()
     }
 
-    fn get_tab_label(nvim: Arc<Mutex<Neovim>>, tab: &Tabpage, tab_name: String) -> gtk::Label {
-        let mut nvim = nvim.lock().unwrap();
-        let tab_label;
-        let mut modified = false;
-
-        // Handle possible errors 
-        if let Ok(win) = tab.get_win(&mut nvim) {
-            if let Ok(buf) = win.get_buf(&mut nvim) {
-                if let Ok(option) = buf.get_option(&mut nvim, "mod") {
-                    if let Some(changed) = option.as_bool() {
-                        modified = changed;
-                    }
-                }
-            }
-        } else {
-            modified = false;
-        }
-
-        // Provide visual indicator if tab buffer is modified
-        if modified {
-            let mut tab_string = tab_name.clone();
-            tab_string.push_str(" +");
-            tab_label = gtk::Label::new(tab_string.as_str());
-        } else {
-            tab_label = gtk::Label::new(tab_name.as_str());
-        }
-
-        tab_label
+    fn get_tab_label(
+        nvim: &mut Neovim,
+        tab: &Tabpage,
+        tab_name: &str,
+    ) -> gtk::Label {
+        let modified = tab
+            .list_wins(nvim)
+            .and_then(|wins| {
+                wins.iter().map(|win| win.get_buf(nvim)).collect()
+            })
+            .and_then(|bufs: Vec<Buffer>| {
+                bufs.iter()
+                    .map(|buf| buf.get_option(nvim, "mod"))
+                    .collect()
+            })
+            .and_then(|vals: Vec<Value>| {
+                Ok(vals
+                    .iter()
+                    // If parsing to bool fails, default to false.
+                    .map(|val| val.as_bool().unwrap_or(false))
+                    .collect())
+            })
+            .and_then(|mods: Vec<bool>| {
+                // If any of the buffes is modified, mark the tab with modified
+                // indicator.
+                Ok(mods.iter().find(|m| **m == true).is_some())
+            })
+            // If something went wrong, default to false.
+            .unwrap_or(false);
+
+        let title = format!("{}{}", tab_name, if modified { " +" } else { "" });
+        gtk::Label::new(title.as_str())
     }
 
-    pub fn update(&self, nvim: Arc<Mutex<Neovim>>, current: Tabpage, tabs: Vec<(Tabpage, String)>) {
+    pub fn update(
+        &self,
+        nvim: &mut Neovim,
+        current: Tabpage,
+        tabs: Vec<(Tabpage, String)>,
+    ) {
         glib::signal_handler_block(&self.notebook, &self.switch_tab_signal);
         for child in self.notebook.get_children() {
             self.notebook.remove(&child);
@@ -114,7 +123,7 @@ impl Tabline {
 
         let mut page = 0;
         for (i, tab) in tabs.iter().enumerate() {
-            let tab_label = Tabline::get_tab_label(nvim.clone(), &tab.0, tab.1.clone());
+            let tab_label = Tabline::get_tab_label(nvim, &tab.0, &tab.1);
             tab_label.set_hexpand(true);
             tab_label.set_ellipsize(pango::EllipsizeMode::End);
             add_css_provider!(&self.css_provider, tab_label);
diff --git a/src/ui/ui.rs b/src/ui/ui.rs
index 5acc202..6285d46 100644
--- a/src/ui/ui.rs
+++ b/src/ui/ui.rs
@@ -714,7 +714,8 @@ fn handle_redraw_event(
                 state.popupmenu.select(*selected as i32, &state.hl_defs);
             }
             RedrawEvent::TablineUpdate(cur, tabs) => {
-                state.tabline.update(nvim.clone(), cur.clone(), tabs.clone());
+                let mut nvim = nvim.lock().unwrap();
+                state.tabline.update(&mut nvim, cur.clone(), tabs.clone());
             }
             RedrawEvent::CmdlineShow(cmdline_show) => {
                 state.cmdline.show(cmdline_show, &state.hl_defs);

Using high-order functions to get the modified value avoids unnecessary nesting. Also passing mutable reference of nvim to Tabline::update reduces reference counting and locking.

Note that I'm currently using list_wins instead of get_win on the tab page; I'm not sure which one is better. What do you think?

Regarding the + vs. more visual indication: I think its worth checking out what other editors do. Some adds */+ or what ever characther they have picked and some seem to add a "dot".

As a reference (notice the tabs):

I kinda prefer the "dot" because that is more "GUI".

[smolck]

Thanks @vhakulinen, will push those changes in a bit (once I get to my computer). Concerning using the get_win function, would that remove any overhead or complexity? If no or not much, I’d say leave it as list_wins (and tbh, I didn’t know you could do half of the stuff you did in that change; I’m still taking in that beautiful Rust code! I don’t want to hurt it.... 😂).

I’m not sure that the dot complements gnvim’s tabline (which is longer & bigger than vscode’s, for example). I like the idea of something that sets gnvim apart from the other Neovim GUIs, whether that be something like what badosu sketched out or something else. What do you think? Again, this isn’t essential for this PR (although if it isn’t much trouble I’d be glad to implement it here, and am not against the idea), so we can always revisit it.

Thanks again for the instructive moment (still learning core Rust and it’s idioms).

[badosu]

I did some experimentation with unicode circles/bullets (● black circle 25cf) :

Might be interesting to test some variations, see: https://stackoverflow.com/questions/12971187/what-would-be-the-unicode-character-for-big-bullet-in-the-middle-of-the-characte

[smolck]

That looks pretty good! If we are going to do something text-based, that is probably the way to go (looks much better than a plus sign). We should maybe look into some other unicode symbols too (and verify that a fairly large amount of fonts support them, especially the popular fonts).

Also, is the additional code that would be necessary to do something css-based worth the look we'd get, or should we stick to text? I haven't tried to actually implement a css-based indication, so I am not sure atm.

[badosu]

I think an initial approach with text is better for an initial iteration, e.g. replacing " +" with " ●" and see if it looks better.

Unfortunately I did not find a circle/bullet character that actually stays in the middle of the line height, not sure if it depends on font.

Anyway, either of + or ● is fine by me.

[smolck]

I think an initial approach with text is better for an initial iteration, e.g. replacing " +" with " ●" and see if it looks better.

Agreed.

Unfortunately I did not find a circle/bullet character that actually stays in the middle of the line height, not sure if it depends on font.

Interesting. Is this important enough that we should stick to using +? I imagine that + doesn’t have that problem?

Anyway, either of + or ● is fine by me.

They definitely both communicate that a tab has a modified buffer, but I think I prefer + after trying out both symbols. If the first iteration is going to be text-based, why not just be conservative and do the same as the Neovim TUI, with +? By doing that we avoid inconsistencies with fonts, and keep things uniform between the TUI and GNvim (for now at least).

What do you think @vhakulinen?

[vhakulinen]

I personally prefer the dot, but we can make the character it self configurable (especially if the dot's position varies between fonts). I'll merge this.