"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Mark Jamsek <mark@jamsek.com>
Subject:
Re: fix cycle view in fullscreen regression
To:
Omar Polo <op@omarpolo.com>
Cc:
gameoftrees@openbsd.org
Date:
Mon, 20 Jun 2022 14:09:36 +1000

Download raw body.

Thread
On 22-06-20 11:50am, Mark Jamsek wrote:
> On 22-06-19 06:20pm, Omar Polo wrote:
> > Mark Jamsek <mark@jamsek.com> wrote:
> > > When in a child diff view in splitscreen, if you toggle fullscreen and
> > > then cycle back to the log view with tab, the log view opens in
> > > splitscreen with the diff view still visible and no border line.
> > > 
> > > repro:
> > >  $ tog
> > >  return
> > >  f
> > >  tab
> > >  # log view opens in a split instead of fullscreen
> > 
> > good catch!
> > 
> > I broke this in 0dbbbe79, apologize, when changing the main view to be
> > resized when entering the split mode.  I didn't know you could cycle
> > fullscreen views, so I missed to test it.
> > 
> > > What used to happen is we would cycle to the log view in fullscreen.
> > > 
> > > The below patch appears to fix this while keeping the hscroll fix in
> > > splitscreen when drawing double-width chars (tested in the ja.git repo).
> > > 
> > > This happens because we now resize views _after_ resetting nlines and
> > > ncols based on the child start column in splitscreen, which sets the
> > > parent view->ncols to the child view's splitscreen start column.
> > > 
> > > The fix checks if the current view is not in a split when cycling views
> > > and, if not, resets fullscreen dimensions, then bypasses resetting
> > > these dimensions when resizing.
> > 
> > I tried to play with an alternative (manually resizing the parent when
> > the child view goes fullscreen) but althought it was simpler, it had its
> > own share of issues and bringed back a glitch with the double width
> > characters.  yours works perfectly! :)
> 
> I tried a few different things but had the same problem: either cycling
> between fullscreen views or the wide character splitscreen would break.
> 
> > However, I don't particularly like how we're adding an extra parameter
> > to view_resize.  It would be nice to simplify view_resize somehow:
> > having it doing "layout decisions" is a bit awkward IMHO, it would be
> > nicer to have these things decided by some higher level code and
> > view_resize acting as dumb as possible.
> > 
> > (to highlight the issue: when we're in fullscreen mode and the terminal
> > is resized, view_resize "decides" that we want to exit fullscreen too.)
> > 
> > I'm not opposing to the patch going in, fixing view_resize may not be
> > straightforward so in the meantime we could fix the issue at least.
> 
> I wasn't a fan either tbh, but the only other fix I had duplicated a bit
> of view_resize(). It's less invasive though and appears to fix the
> problem without breaking the double-width glitch either. Same logic as
> the previous diff but moved up to view_fullscreen().
> 
> diff d8b5af438b16bcea5568b1d4bfc127567e35e2f6 /Users/mark/Library/Mobile Documents/com~apple~CloudDocs/src/got-current
> blob - b8935d8f4d93a4f03c1d8727adb23a6137741acf
> file + tog/tog.c
> --- tog/tog.c
> +++ tog/tog.c
> @@ -732,9 +732,17 @@ view_fullscreen(struct tog_view *view)
>  	view->ncols = COLS;
>  	view->lines = LINES;
>  	view->cols = COLS;
> -	err = view_resize(view);
> -	if (err)
> -		return err;
> +	if (!view->child) {
> +		err = view_resize(view);
> +		if (err)
> +			return err;
> +	} else {
> +		if (wresize(view->window, view->nlines, view->ncols) == ERR)
> +			return got_error_from_errno("wresize");
> +		if (replace_panel(view->panel, view->window) == ERR)
> +			return got_error_from_errno("replace_panel");
> +		wclear(view->window);
> +	}
> 
>  	if (mvwin(view->window, view->begin_y, view->begin_x) == ERR)
>  		return got_error_from_errno("mvwin");
> @@ -956,6 +964,8 @@ view_input(struct tog_view **new, int *done, struct to
>  			view->focussed = 0;
>  			view->parent->focussed = 1;
>  			view->parent->focus_child = 0;
> +			if (!view_is_splitscreen(view))
> +				err = view_fullscreen(view->parent);
>  		}
>  		break;
>  	case 'q':
> 
> 

Actually, I think this is better. And like the other problem you
noticed, view_resize() now also honours fullscreen when resizing.

diff d8b5af438b16bcea5568b1d4bfc127567e35e2f6 /Users/mark/Library/Mobile Documents/com~apple~CloudDocs/src/got-current
blob - b8935d8f4d93a4f03c1d8727adb23a6137741acf
file + tog/tog.c
--- tog/tog.c
+++ tog/tog.c
@@ -748,6 +748,13 @@ view_is_parent_view(struct tog_view *view)
 	return view->parent == NULL;
 }

+static int
+view_is_splitscreen(struct tog_view *view)
+{
+	return view->begin_x > 0;
+}
+
+
 static const struct got_error *
 view_resize(struct tog_view *view)
 {
@@ -763,7 +770,7 @@ view_resize(struct tog_view *view)
 	else
 		ncols = view->ncols + (COLS - view->cols);

-	if (view->child) {
+	if (view->child && view_is_splitscreen(view->child)) {
 		view->child->begin_x = view_split_begin_x(view->begin_x);
 		if (view->child->begin_x == 0) {
 			ncols = COLS;
@@ -818,12 +825,6 @@ view_set_child(struct tog_view *view, struct tog_view
 	return view_resize(view);
 }

-static int
-view_is_splitscreen(struct tog_view *view)
-{
-	return view->begin_x > 0;
-}
-
 static void
 tog_resizeterm(void)
 {
@@ -956,6 +957,8 @@ view_input(struct tog_view **new, int *done, struct to
 			view->focussed = 0;
 			view->parent->focussed = 1;
 			view->parent->focus_child = 0;
+			if (!view_is_splitscreen(view))
+				err = view_fullscreen(view->parent);
 		}
 		break;
 	case 'q':


-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68