Re: [screen-devel] Window group code contribution

[Amadeusz Sławiński]

On Wed, 7 Nov 2018 00:07:59 +0100
Amadeusz Sławiński <address@hidden> wrote:

> On Fri, 2 Nov 2018 00:04:20 -0500
> Ethan Warth <address@hidden> wrote:
> 
> > I use window groups in my .screenrc.  Window groups, as you likely already 
> > know,
> > can be used to group other child windows (including other child window 
> > groups)
> > together.  Window groups do /not/ have their own corresponding shell, 
> > though;
> > when you switch to a window group's window, you instead see the output of a
> > windowlist command (as a consequence, that group window's children are 
> > listed,
> > and can then be selected from).
> > 
> > Windowlist has two toggles that affect its output: the list can be sorted in
> > either index or MRU order, and the list can contain either just immediate
> > children windows of all descendant windows.  Since window groups use the 
> > same
> > code for their own output, they also support these toggles.
> > 
> > However, in the current code base, these toggles are reset to index 
> > ordering and
> > display of immediate children only every time a particular window group is
> > swapped out of and back into a pane.  To instead make these display settings
> > persist under such conditions on a per-window group basis, I made the
> > following code changes:
> > 
> > * I added two state booleans to the window structure to represent MRU 
> > ordering
> > and recursive descendant listing for window groups (and added values for 
> > these
> > two booleans to statically defined window structs in the code).
> > * I made the windowlist initialization function ignore the passed in order 
> > flags
> > if the windowlist is being generated for a window group, and I made the 
> > function
> > instead use the window group's stored flags.
> > * I made the sections of code that change the aforementioned flags for an 
> > active
> > windowlist also change those flags for the corresponding window group (in 
> > the
> > case that one exists, i.e., when the windowlist is being displayed as the
> > window group's output on a pane).
> > 
> > I tried to make the commit title sufficiently descriptive for the change, 
> > but
> > I'm open to alternatives for such a description.  I have also confirmed the
> > trailing whitespace instances that you have alluded to in your response.  
> > I'm
> > more than happy to submit modified patches to reflect both the potential
> > commit description change and the removal of the trailing whitespaces; just
> > let me know what (if anything) the first commit's description should be
> > changed to.
> > 
> > 
> > Perfect normality is impossible.  Be unique!
> >    ―redyoshi49q
> > 
> > 
> > On Thu, Nov 1, 2018 at 3:08 PM Amadeusz Sławiński <address@hidden> wrote:  
> > >
> > > On Mon, 22 Oct 2018 20:21:16 -0500
> > > Ethan Warth <address@hidden> wrote:
> > >    
> > > > (I'm resending this, to the mailing list this time.  I didn't notice 
> > > > that my
> > > > reply wasn't actually going to the list.)
> > > >
> > > >
> > > > Sorry; your message ended up in my spam folder, and I didn't notice it
> > > > immediately.
> > > >
> > > > I will be including the git format-patch (both as inline text in this 
> > > > message
> > > > and also as file attachments as a redundancy measure).  This is my 
> > > > first time
> > > > submitting git patches over email; let me know if I need to make another
> > > > attempt at sending the commit patches.
> > > >    
> > >
> > > Hi,
> > >
> > > sorry for delay, few things:
> > >
> > > Can you describe, what is a goal of first patch? I've read it few times
> > > and am a bit unsure. You can describe workflow which it fixes,
> > > preferably it should be also included in commit message, so if someone
> > > looks at commit later they know what it is about.
> > >
> > > There also seems to be few leftover whitespace characters at end of
> > > lines in first and second patch, you should be able to see them using
> > > "git show" command, they will be colored in red.
> > >
> > > Cheers,
> > > Amadeusz    
> 
> Hi,
> 
> Yes, I see now. This should be described in commit message, it's not
> clear from topic alone what the patch is about.
> 
> At least those lines should be added to commit message:
> > Windowlist has two toggles that affect its output: the list can be sorted in
> > either index or MRU order, and the list can contain either just immediate
> > children windows of all descendant windows.  Since window groups use the 
> > same
> > code for their own output, they also support these toggles.
> > 
> > However, in the current code base, these toggles are reset to index 
> > ordering and
> > display of immediate children only every time a particular window group is
> > swapped out of and back into a pane.   
> 
> As for the patch itself, it looks good, but I think there can be few
> improvements.
> Instead of naming variables "w_list_mru" and "w_list_group "
> name them after variables they are correspong to, so: "w_list_order" and
> "w_list_nested".
> 
> Also comments in patch:
> 
> diff --git a/src/list_window.c b/src/list_window.c
> index 620f706..a6a5d02 100644
> --- a/src/list_window.c
> +++ b/src/list_window.c
> @@ -278,6 +278,8 @@ static int gl_Window_input(ListData *ldata, char
> **inp, size_t *len)
>      case 'm':
>          /* Toggle MRU-ness */
>          wdata->order = wdata->order == WLIST_MRU ? WLIST_NUM : WLIST_MRU;
> +        if (wdata->group)
> +            wdata->group->w_list_mru = wdata->order == WLIST_MRU;
> 
> Also here make assingment similar to the one for wdata->order few lines above.
> 
>          glist_remove_rows(ldata);
>          gl_Window_rebuild(ldata);
>          break;
> @@ -285,6 +287,8 @@ static int gl_Window_input(ListData *ldata, char
> **inp, size_t *len)
>      case 'g':
>          /* Toggle nestedness */
>          wdata->nested = !wdata->nested;
> +        if (wdata->group)
> +            wdata->group->w_list_group = wdata->nested ? true : false;
>          glist_remove_rows(ldata);
>          gl_Window_rebuild(ldata);
>          break;
> @@ -453,8 +457,11 @@ void display_windows(int onblank, int order, Window 
> *group)
>          return;
>      }
> 
> -    if (group)
> +    if (group) {
>          onblank = 0;    /* When drawing a group window, ignore 'onblank' */
> +        order = (group->w_list_group ? WLIST_NESTED : 0) +
> +            (group->w_list_mru ? 1 : 0); /* also ignore order flags */
> 
> In case of window order use defines here:  ? WLIST_MRU : WLIST_NUM
> Also added comment is bit confusing, because you are not actually
> ignoring order flags, but rather restoring them
> 
> +    }
> 
>      if (onblank) {
>          if (!display) {
> diff --git a/src/window.c b/src/window.c
> index e197390..b6d99fb 100644
> --- a/src/window.c
> +++ b/src/window.c
> @@ -95,6 +95,8 @@ struct NewWindow nwin_undef = {
>      .aflag               = false,
>      .dynamicaka          = false,
>      .flowflag            = -1,
> +    .list_mru            = false,
> +    .list_group          = false,
>      .lflag               = -1,
>      .histheight          = -1,
>      .monitor             = -1,
> @@ -121,6 +123,8 @@ struct NewWindow nwin_default = {
>      .aflag      = false,
>      .dynamicaka = true,
>      .flowflag   = FLOW_ON,
> +    .list_mru   = false,
> +    .list_group = false,
>      .lflag      = 1,
>      .histheight = DEFAULTHISTHEIGHT,
>      .monitor    = MON_OFF,
> @@ -153,6 +157,8 @@ void nwin_compose(struct NewWindow *def, struct
> NewWindow *new, struct NewWindow
>      COMPOSE(aflag);
>      COMPOSE(dynamicaka);
>      COMPOSE(flowflag);
> +    COMPOSE(list_mru);
> +    COMPOSE(list_group);
>      COMPOSE(lflag);
>      COMPOSE(histheight);
>      COMPOSE(monitor);
> @@ -556,6 +562,8 @@ int MakeWindow(struct NewWindow *newwin)
>      p->w_savelayer = &p->w_layer;
>      p->w_pdisplay = NULL;
>      p->w_lastdisp = NULL;
> +    p->w_list_mru = nwin.list_mru;
> +    p->w_list_group = nwin.list_group;
> 
>      if (display && !AclCheckPermWin(D_user, ACL_WRITE, p))
>          p->w_wlockuser = D_user;
> diff --git a/src/window.h b/src/window.h
> index e6faaee..8989de3 100644
> --- a/src/window.h
> +++ b/src/window.h
> @@ -50,6 +50,8 @@ struct NewWindow {
>      bool    aflag;
>      bool    dynamicaka;
>      int    flowflag;
> +    bool    list_mru;    /* MRU list order for window groups */
> +    bool    list_group; /* show nested children in window groups */
> 
> Those comments seem misaligned when I apply patch.
> 
>      int    lflag;
>      int    histheight;
>      int    monitor;
> @@ -139,6 +141,8 @@ struct Window {
>      Window *w_next;            /* next window */
>      Window *w_prev_mru;        /* previous most recently used window */
>      int    w_type;            /* type of window */
> +    bool w_list_mru;    /* MRU list order for window groups */
> +    bool w_list_group;    /* show nested children in window groups */
> 
> Those also are misaligned.
> 
>      Layer w_layer;            /* our layer */
>      Layer *w_savelayer;        /* the layer to keep */
>      int    w_blocked;        /* block input */
> --
> 2.17.0
> 
> Cheers,
> Amadeusz
> 

Did the fixes myself and pushed to master.
Thanks for patches!

Amadeusz