Re: [screen-devel] Window group code contribution

[Amadeusz Sławiński]

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