Toward enable `should_render` (fix pane render) #318

horasal · 2021-04-22T07:35:16Z

This pr is the first step toward enable should_render, by fixing pane render when switching/resize tab.

When should_render is enabled, pane won't be re-rendered unless its content is changed. When switching tab, panes in target tab may be not changed from its last render. Thus the render requests will be ignored and those panes' areas will mess up.

This pr add a set_force_render to Tab, which will clear the should_render flag of panes belongs to this tab. Then tab::rander call can rerender all panes.

currently, set_force_render is called when:

go to tab
switch tab next
switch tab prev
terminal resize

pane should_render will be true if

reflow_lines happens
new character is written to terminal
scroll

horasal · 2021-04-22T08:00:14Z

All tests passed after enable should_render and remove cfg(test), results can be found at actions for commit d949a99

After that, 8c47401 changes should_render() to true because we still have wide char issue.
aa25ba2 fixes cargo fmt and cargo clippy.

imsnif

Hey, @horasal - great work on this. I very much like this solution. Thanks for being patient with the review taking a bit longer than usual with the burst of attention this project received in the past week :)

Personally, I'm good to go forward with this. But I'd first like to understand the plan: are you interested in continuing the work on this? As I understand it, the main thing we have left is dealing with the implications of wide-characters on this. Do you have a plan to deal with them? I have some ideas otherwise... let me know if/how you'd like to continue. Then we can think if we'd like to have that change as part of this PR or not (eg. there are a few hacky ideas I can think of that can get us to uncomment the conditional in this PR and move forward).

imsnif · 2021-04-26T08:28:06Z

src/client/tab.rs

@@ -656,6 +656,11 @@ impl Tab {
    pub fn toggle_fullscreen_is_active(&mut self) {
        self.fullscreen_is_active = !self.fullscreen_is_active;
    }
+    pub fn set_force_render(&mut self) {


Nice. I would have gone with something on the state of Tab that bypasses the should_render of each pane, but the more I think about it, the more I like this solution.

imsnif · 2021-04-26T08:30:22Z

src/client/panes/terminal_pane.rs

@@ -315,6 +316,7 @@ impl TerminalPane {
        let rows = self.get_rows();
        let columns = self.get_columns();
        self.grid.change_size(rows, columns);
+        self.set_should_render(true);


horasal · 2021-04-26T23:07:12Z

are you interested in continuing the work on this? As I understand it, the main thing we have left is dealing with the implications of wide-characters on this. Do you have a plan to deal with them?

I did some experiments but still need many works and far from becoming a PR.

horasal@e8e8639

(In current zellij, grid and cursor treat number of character = width of string = position. However for widechar, width can be larger than character so this rule doesn't apply. I'm not sure a major revise on grid is the right way or not)

I have some ideas otherwise

It will be great if you can make zellij works with widechar!

imsnif · 2021-04-27T07:18:08Z

For sure making Zellij support wide characters is on the roadmap, but as you say this requires a bigger change.
I was thinking we can improve on the current functionality (namely, uncomment that if), if we do the following:

Identify wide characters as part of the TerminalCharacter struct
Force a tab re-render if there's a wide character anywhere in the viewport.

A little hacky, but I think it's better than what we have now and will probably give us a performance boost until we can handle them properly. What do you think?

horasal · 2021-04-27T22:02:33Z

It sounds good so I implemented it: https://github.com/horasal/zellij/tree/widechar_force_render
but panes still messed up... Did I miss something?

imsnif · 2021-04-28T13:14:33Z

It sounds good so I implemented it: https://github.com/horasal/zellij/tree/widechar_force_render
but panes still messed up... Did I miss something?

Unfortunately, you're not missing anything - this happens in main too :D

Since we don't support wide unicode characters yet, we don't properly detect them in order to line wrap inside a pane. So they end up being line-wrapped by the terminal emulator itself.

This issue is super high priority for us to fix, and the only reason I didn't get to it myself yet is that there are other super high priority fixes on my plate before it :)

That being said, I think it should not interfere with this PR... I'm happy you implemented this idea! Is it ready for a review?

horasal · 2021-04-29T06:19:39Z

That being said, I think it should not interfere with this PR... I'm happy you implemented this idea! Is it ready for a review?

I agree with you. This pr should not be blocked by widechar problem. This is ready for a review.

imsnif · 2021-04-29T16:23:20Z

Hey, this is great! I'm good to go with this, but I would prefer to go with your solution of the wide characters and then uncomment the if true line in the render function. What do you think?

horasal · 2021-04-30T03:19:01Z

merge widechar_force_render, and uncomment if true in pane render func.

imsnif · 2021-04-30T14:01:02Z

Thank you very much for your work on this!

fix pane render when switch/resize tab

4c94f57

horasal marked this pull request as draft April 22, 2021 07:39

horasal added 3 commits April 22, 2021 16:49

render when reflow_lines

d949a99

back to always render because widechar issue

8c47401

fix clippy

aa25ba2

horasal marked this pull request as ready for review April 22, 2021 08:00

TheLostLambda requested a review from imsnif April 23, 2021 02:53

imsnif reviewed Apr 26, 2021

View reviewed changes

horasal added 2 commits April 28, 2021 06:47

force_render when current grid contains widechar

e1f7c72

bug fix

05e150a

pane_contains -> panes_contain

98c5ec4

imsnif mentioned this pull request Apr 28, 2021

Output buffer #374

Closed

horasal and others added 2 commits April 29, 2021 18:09

Merge branch 'main' into fix_tab_render

0bd52c2

fix conflict

6bd2dcf

Merge branch 'widechar_force_render' into fix_tab_render

b5f4408

fix(terminal): bring back should_render

392a7d6

imsnif merged commit 454ad0e into zellij-org:main Apr 30, 2021

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toward enable `should_render` (fix pane render) #318

Toward enable `should_render` (fix pane render) #318

horasal commented Apr 22, 2021 •

edited

Loading

horasal commented Apr 22, 2021 •

edited

Loading

imsnif left a comment

imsnif Apr 26, 2021

imsnif Apr 26, 2021

horasal commented Apr 26, 2021

imsnif commented Apr 27, 2021

horasal commented Apr 27, 2021

imsnif commented Apr 28, 2021 •

edited

Loading

horasal commented Apr 29, 2021

imsnif commented Apr 29, 2021

horasal commented Apr 30, 2021

imsnif commented Apr 30, 2021

Toward enable should_render (fix pane render) #318

Toward enable should_render (fix pane render) #318

Conversation

horasal commented Apr 22, 2021 • edited Loading

horasal commented Apr 22, 2021 • edited Loading

imsnif left a comment

Choose a reason for hiding this comment

imsnif Apr 26, 2021

Choose a reason for hiding this comment

imsnif Apr 26, 2021

Choose a reason for hiding this comment

horasal commented Apr 26, 2021

imsnif commented Apr 27, 2021

horasal commented Apr 27, 2021

imsnif commented Apr 28, 2021 • edited Loading

horasal commented Apr 29, 2021

imsnif commented Apr 29, 2021

horasal commented Apr 30, 2021

imsnif commented Apr 30, 2021

Toward enable `should_render` (fix pane render) #318

Toward enable `should_render` (fix pane render) #318

horasal commented Apr 22, 2021 •

edited

Loading

horasal commented Apr 22, 2021 •

edited

Loading

imsnif commented Apr 28, 2021 •

edited

Loading