[WIP] Fix #85

[smolck]

This is fairly close to working, but it has visual artifacts (and the font looks a bit blurred to me, but maybe not for everyone?):

Interestingly, they disappear after opening the cmdline (so after typing :):

The line that's causing this "reset" of the artifacts is here (see #85 (comment)):

gnvim/src/ui/cmdline.rs

Line 575 in 65846f4

self.fixed.show_all();

But I have no idea why calling show_all on a gtk::Fixed removes the artifacts. Any ideas?

The artifacts appear to be coming from a lack of subpixel positioning in GNvim, but I could be wrong. Note that when I ceil (or floor) the width value of the CellMetrics the artifacts disappear, but the cursor is no longer positioned correctly. (All this still doesn't explain the "reset" when opening the cmdline though.)

cc: @vhakulinen @last-partizan

[last-partizan]

With this fixes blurring on scroll is gone, and i see almost no artifacts.

Hovewer, artifacts appear when moving cursor from end to start in this line:

It looks like new bug, with master branch it works without artifacts.

[vhakulinen]

Some code in render.rs is casting f64 to i32. My guess is that that is causes those artifacts with cursor movement.

[smolck]

Some code in render.rs is casting f64 to i32. My guess is that that is causes those artifacts with cursor movement.

Would you mind pointing out the code you're speaking of here?

[vhakulinen]

Sorry, the cursor stuff was in grid.rs:

gnvim/src/ui/grid/grid.rs

Lines 327 to 352 in 65846f4

	pub fn cursor_goto(&self, row: u64, col: u64) {
	let mut ctx = self.context.borrow_mut();
	let ctx = ctx.as_mut().unwrap();
	// Clear old cursor position.
	let (x, y, w, h) = ctx.get_cursor_rect();
	ctx.queue_draw_area
	.push((x as i32, y as i32, w as i32, h as i32));
	ctx.cursor.0 = row;
	ctx.cursor.1 = col;
	// Mark the new cursor position to be drawn.
	let (x, y, w, h) = ctx.get_cursor_rect();
	ctx.queue_draw_area
	.push((x as i32, y as i32, w as i32, h as i32));
	if let Some(ref im_context) = self.im_context {
	let rect = gdk::Rectangle {
	x: x as i32,
	y: y as i32,
	width: w as i32,
	height: h as i32,
	};
	im_context.set_cursor_location(&rect);
	}
	}

[smolck]

@vhakulinen Is there an alternative to gdk::Rectangle we could use that takes floats? There isn't really a way to avoid the casting to i32 without changing that because a gdk::Rectangle takes i32s.

[vhakulinen]

Probably not, but I'm not sure of its significance for the im_context. The ones for queue_draw_area should be rounded up and cast to i32.

[smolck]

The ones for queue_draw_area should be rounded up and cast to i32.

@vhakulinen Thanks! I've pushed your changes, and that's removed the worst of the artifacts. The only artifacts I saw some artifacts in my short test when opening a file with vim-startify, so I'm not sure what's causing that. In addition, I've noticed artifacts sometimes when opening new files. Otherwise it works great for me!

@last-partizan Could you test this branch again and see if it works for you?

EDIT: I've noticed other cases with artifacts, so this PR isn't as ready as I thought. Modified comment accordingly.

[last-partizan]

@smolck Looks like it still has artifacts after cursor movement for me.

[smolck]

@smolck Looks like it still has artifacts after cursor movement for me.

Interesting. I'm not getting artifacts on cursor movement, do they look the same (and occur in the same way) as those you saw in #124 (comment)?

[smolck]

@last-partizan I've pushed a commit ceiling the values to queue_draw_area in context.rs, and it's appears to have removed all of the artifacts for me. Would you mind testing this branch again with the latest commit?

[last-partizan]

Still don't work on my laptop, wait a minute i will check on desktop.

[smolck]

Still don't work on my laptop, wait a minute i will check on desktop.

Okay, thanks! What version of pango are you using on your laptop? I'm using version 1:1.44.7-1.

[last-partizan]

@smolck same version.

Doesn't work on desktop either.

[last-partizan]

https://imgur.com/0cF0Wow

here is how it looks

[last-partizan]

I was thinking, maybe it just on wayland, but no. Tried under xorg, same result.

[smolck]

Hmm, that's strange. I don't get any artifacts now. I wonder why it works on my machine and not yours.....

@liyiheng In #85 (comment) you mentioned you had issues with pango and GNvim. Can you test this branch and see if it fixes the problems you've had?

[smolck]

@last-partizan I guess I wasn't testing things enough, I'm getting the artifacts your seeing too. I'm going to see if I can fix/improve that. Thanks for the info.

[smolck]

It's interesting how the artifacts appear almost exclusively when moving the cursor to the right, and how the artifacts disappear as I move the cursor back to the left (so that the cursor goes over the artifacts).

[smolck]

Also interesting is that if I do :set guifont=Iosevka\ Extrabold:h14.5 so that I have a floating-point font size, not as many artifacts appear when moving the cursor, or at least the artifacts are more spread out.

[liyiheng]

@smolck
pango 1:1.44.7-1
Commit 001d4ff

I tested H,J,K,L.
H,J,K are fine, but L:

[smolck]

I tested H,J,K,L.
H,J,K are fine, but L . . .

@vhakulinen Any idea what could be causing the artifacts only when moving the cursor with L?

[vhakulinen]

Hmh, could be the Grid::tick function. It should also ceil the values before casting to i32.

[smolck]

@vhakulinen Still have artifacts when ceiling the values pushed to queue_draw_area at the end of Grid::tick, both with and without :set guicursor += a:blinkon333.

[vhakulinen]

Maybe you need to floor the x values.

[smolck]

I was able to remove the cursor-caused artifacts on my machine by ceiling the values used for drawing the cursor in drawingarea_draw(). I think the artifacts were caused by the values for the cursor's rectangle's size being inconsistent between queue_draw_area and the drawingarea_draw() function. So, after ceiling the cursor rectangle values used in drawingarea_draw the artifacts no longer occur, at least for me.

@last-partizan @liyiheng Can you guys try this branch once more with the latest commit I just pushed? The cursor should no longer cause any artifacts. Thank you for testing this PR btw, doing so helps out a lot.

@vhakulinen Do you think this PR is now good for merge (assuming there are no more problems for last-partizan or liyiheng), or is there anything we should/can clean up or polish?

I'd like to at least make the ceiling of the values passed to queue_draw_area happen in one place, by creating a function to do so or adapting existing functions to return ceiled f64s as i32s. That way, we (1) don't repeat ourselves and (2) make it easier to maintain consistency throughout the code.

[liyiheng]

Commit 150b1a7
@smolck Everything is OK on my laptop

[smolck]

@vhakulinen I've pushed b383efd, which I think is a good first step for making this code cleaner, but I think render.rs still needs some work. I've removed the [WIP] label from the PR in any case, since this is now ready for review and polish, and not really a "work-in-progress" anymore.

[last-partizan]

I can confirm, artifacts under cursor are gone.

But one little is still present is statusbar:

There is thin vertical lines, and they are gone after area is repainted (for example by changing vim mode to edit and back)

[smolck]

Yeah, I thought I noticed an artifact in the statusbar, but I didn’t look into it (nor did I realize more were present or could be). Thanks for the info, I’ll see what I can come up with. Back to WIP! ;)

[smolck]

Interesting. So if I change this:

gnvim/src/ui/grid/context.rs

Line 213 in b383efd

fm.get_approximate_char_width() as f64 / pango::SCALE as f64;

into this:

fm.get_approximate_digit_width() as f64 / pango::SCALE as f64; 

Then the problem with the lines in the statusline is fixed. But then the text inside the editor is too close together in some places, like in this example:

Any ideas?

cc: @vhakulinen

[smolck]

@vhakulinen It feels wrong and hacky to just ceil values everywhere, rather than find a way to properly support floating-point CellMetrics; I think we should try to fix the issues without ceiling values.

Here's a starting diff for this, based off of master:

diff --git a/src/ui/grid/context.rs b/src/ui/grid/context.rs
index e7ca63b..8844577 100644
--- a/src/ui/grid/context.rs
+++ b/src/ui/grid/context.rs
@@ -43,7 +43,7 @@ pub struct Context {
     pub active: bool,
 
     /// Areas to call queue_draw_area on the drawing area on flush.
-    pub queue_draw_area: Vec<(i32, i32, i32, i32)>,
+    pub queue_draw_area: Vec<(f64, f64, f64, f64)>,
 }
 
 impl Context {
@@ -207,7 +207,7 @@ impl CellMetrics {
         self.ascent = fm.get_ascent() as f64 / pango::SCALE as f64 + extra;
         self.decent = fm.get_descent() as f64 / pango::SCALE as f64 + extra;
         self.height = self.ascent + self.decent;
-        self.width = (fm.get_approximate_digit_width() / pango::SCALE) as f64;
+        self.width = fm.get_approximate_char_width() as f64 / pango::SCALE as f64;
 
         self.underline_position =
             fm.get_underline_position() as f64 / pango::SCALE as f64 - extra;
diff --git a/src/ui/grid/grid.rs b/src/ui/grid/grid.rs
index a9e2b04..65f9938 100644
--- a/src/ui/grid/grid.rs
+++ b/src/ui/grid/grid.rs
@@ -137,7 +137,7 @@ impl Grid {
         ctx.cursor_color = hl.foreground.unwrap_or(hl_defs.default_fg);
 
         while let Some(area) = ctx.queue_draw_area.pop() {
-            self.da.queue_draw_area(area.0, area.1, area.2, area.3);
+            self.da.queue_draw_area(area.0 as i32, area.1 as i32, area.2 as i32, area.3 as i32);
         }
     }
 
@@ -331,14 +331,14 @@ impl Grid {
         // Clear old cursor position.
         let (x, y, w, h) = ctx.get_cursor_rect();
         ctx.queue_draw_area
-            .push((x as i32, y as i32, w as i32, h as i32));
+            .push((x, y, w, h));
         ctx.cursor.0 = row;
         ctx.cursor.1 = col;
 
         // Mark the new cursor position to be drawn.
         let (x, y, w, h) = ctx.get_cursor_rect();
         ctx.queue_draw_area
-            .push((x as i32, y as i32, w as i32, h as i32));
+            .push((x, y, w, h));
 
         if let Some(ref im_context) = self.im_context {
             let rect = gdk::Rectangle {
diff --git a/src/ui/grid/render.rs b/src/ui/grid/render.rs
index c4359b4..9607b42 100644
--- a/src/ui/grid/render.rs
+++ b/src/ui/grid/render.rs
@@ -139,7 +139,7 @@ pub fn cursor_cell(
 fn put_segments(
     cr: &cairo::Context,
     pango_context: &pango::Context,
-    queue_draw_area: &mut Vec<(i32, i32, i32, i32)>,
+    queue_draw_area: &mut Vec<(f64, f64, f64, f64)>,
     cm: &CellMetrics,
     hl_defs: &HlDefs,
     segments: Vec<Segment>,
@@ -159,7 +159,7 @@ fn put_segments(
         let text = seg.leaf.text();
         render_text(cr, pango_context, cm, &hl, hl_defs, &text, x, y, w, h);
 
-        queue_draw_area.push((x as i32, y as i32, w as i32, h as i32));
+        queue_draw_area.push((x, y, w, h));
     }
 }
 
@@ -227,7 +227,7 @@ pub fn clear(da: &DrawingArea, ctx: &mut Context, hl_defs: &HlDefs) {
     cr.fill();
     cr.restore();
 
-    ctx.queue_draw_area.push((0, 0, w, h));
+    ctx.queue_draw_area.push((0.0, 0.0, w as f64, h as f64));
 }
 
 /// Scrolls contents in `ctx.cairo_context` and `ctx.rows`, based on `reg`.
@@ -305,7 +305,7 @@ pub fn scroll(ctx: &mut Context, hl_defs: &HlDefs, reg: [u64; 4], count: i64) {
     cr.rectangle(x1, y1, w, h);
     cr.fill();
     ctx.queue_draw_area
-        .push((x1 as i32, y1 as i32, w as i32, h as i32));
+        .push((x1, y1, w, h));
 
     // Clear the area that is left "dirty".
     let (x1, y1, x2, y2) = get_rect(
@@ -322,7 +322,7 @@ pub fn scroll(ctx: &mut Context, hl_defs: &HlDefs, reg: [u64; 4], count: i64) {
     cr.set_source_rgb(bg.r, bg.g, bg.b);
     cr.fill();
     ctx.queue_draw_area
-        .push((x1 as i32, y1 as i32, w as i32, h as i32));
+        .push((x1, y1, w, h));
 
     cr.restore();
 }

Changing self.width = (fm.get_approximate_digit_width() / pango::SCALE) as f64; to self.width = fm.get_approximate_char_width() as f64 / pango::SCALE as f64; fixed the issues with text being too close together and the cursor not being in the correct position, but there are still artifacts and blurry scrolling.

Blurry scrolling means the scroll() function in render.rs will probably need some tweaking, although I'm not sure what tweaking is needed (I'm unfamiliar with how scrolling works).

Note that while we were working with f64s before pango 1.44, the values ended in .0, whereas now they do not (see #85 (comment)). I think this is what's causing the blurry scrolling, but I'm not sure of the fix. It does appear to be related to the font height, because as @last-partizan pointed out in #85 ceiling the height in CellMetrics fixes the blurry scrolling.

I think making queue_draw_area in Context a Vec<(f64, f64, f64, f64)> instead of a Vec<(i32, i32, i32, i32)> will help with floating-point support, so the above diff does that too.

[vhakulinen]

I'll have a look at this once I find the time.

[vhakulinen]

Anyone having probmels with pango 1.44, can you try https://github.com/vhakulinen/gnvim/tree/fix-pango144?

[elihunter173]

The issues with fuzzy text when scrolling and the cursor not moving the width of the characters seems to be resolved for me on pango 1:1.44.7-1 (Arch Linux).

However, now the grid seems to think that it can fit more characters than it can. In the image below, you can see my status bar isn't shown and there are 14 a's off the screen.

Also, I couldn't get a good screenshot, but bold text seems to have a slightly different width than non-bold text. Causing a very minor jitter when I was searching things with fzf.vim.

Side note, I really appreciate the work you and smolck are doing, and I look forward to be able to use gnvim as my daily driver again! :)

[elihunter173]

Fixed the width issue (as far as I can tell at least). There's times where the grid don't fully span the window because it can't expand by a full cell, but that's expected behavior AFAIK. It was just getting some pretty bad rounding error because of converting width (and height) from a float to an integer and then doing division with integers rather than doing division with floats and then converting to an integer.

I haven't uploaded the commit anywhere, but here's the diff

diff --git a/src/ui/grid/grid.rs b/src/ui/grid/grid.rs
index 2c8e291..a3b176e 100644
--- a/src/ui/grid/grid.rs
+++ b/src/ui/grid/grid.rs
@@ -364,17 +364,17 @@ impl Grid {
         }
     }

-    /// Calcualtes the current size of the grid.
+    /// Calculates the current size of the grid in cells.
     pub fn calc_size(&self) -> (i64, i64) {
         let mut ctx = self.context.borrow_mut();
         let ctx = ctx.as_mut().unwrap();

         let w = self.da.get_allocated_width();
         let h = self.da.get_allocated_height();
-        let cols = (w / ctx.cell_metrics.width as i32) as i64;
-        let rows = (h / ctx.cell_metrics.height as i32) as i64;
+        let cols = w as f64 / ctx.cell_metrics.width;
+        let rows = h as f64 / ctx.cell_metrics.height;

-        (cols, rows)
+        (cols as i64, rows as i64)
     }

     pub fn resize(&self, width: u64, height: u64) {

[smolck]

@vhakulinen Your branch fixes the issues for me (!), except for the problem @elihunter173 stated above. Unfortunately, his patch does not fix the statusline problem for me.

However, the issue with rounding values is that things aren't quite what they should be. Most notably is the status bar. With pango 1.44 and the fix-pango144 branch (with elihunter's patch applied) the (entire) bottom of GNvim looks like this (note the gap between the statusbar and the bottom of the window):

On master branch however, GNvim looks like this (notice the statusbar is almost touching the bottom of the window):

This is not necessarily a deal-breaker for me, but I prefer the look of the statusline without that gap (note that this probably won't matter without the external cmdline enabled since a gap is necessary for the cmdline, but with the external cmdline enabled there's no reason for there to be such a gap).

EDIT: Sorry about the lack of contrast between the wallpaper and the GNvim window in the photos above. The difference is best seen by viewing the images in their own browser tabs. I can re-post the images with a different background if necessary (just let me know).

[elihunter173]

After more testing, I can confirm my patch doesn't fix the bug as @smolck mentioned.

[smolck]

Maybe we should wait on a new release for the Rust bindings to Pango? I wonder if that could have an effect on whether or not sub-pixel positioning is enabled; from my debugging in #85, I found that even after they changed sub-pixel positioning’s default to off upstream in pango 1.44, the CellMetrics (defined in src/ui/grid/context.rs iirc) still had different numbers than those before pango 1.44, and those new numbers had several decimal places of precision they didn’t previously have (e.g., using arbitrary example numbers,10.76755342 for the cell height in pango 1.44 versus 10.0 in pango 1.43).

See gtk-rs/pango#165. At the very least, updated pango bindings would give us more functions to use to try to replicate GNvim’s pre-pango 1.44 behavior.

We can also explore the commit history and source code from other applications using pango and see what those apps did to fix the pango issues on their end.

[smolck]

Note also this could at least partly be an upstream issue with cairo (or pango?), see harfbuzz/harfbuzz#1892 (comment) (and also the whole thread).

Of course there’s also the chance I’m totally wrong about what’s causing the issues ;) But it seems to me like we’re on the right track to a fix, we just need to figure out the proper implementation.

[vhakulinen]

Merged in 13bd8b0