have you tried with libwnck/tasklist.c?

Created attachment 48951 [details] [review]
the patch i'm speaking about

that's my first patch ever, so please read it and give me feedback :-)

the patch is tested only with the test-tasklist script because it's difficult
for me to test it with de gnome-applet without breaking my distrib.

i've build it against libwnck-2.10.0, if cvs version needed please tell me.

line 25 of the patch:

-      /* courent window found:
+      /* current window found:

sorry :)

i'm testing the patch in action with the window-list applet...
seems to work very well, but i've a strange bug when passing over
beep-media-player :

when b-m-p is the only window in this desktop, scrolling minimize the window...
and when there is others windows it scrolls well over others but when comming on
b-m-p it can't go to another (it stays on b-m-p)... can't understand why...

any suggestion ?

ok ! after some testing I think the bug isn't my fault. When you have a
beep-media-player in the tasklist and just click on it in the panel b-m-p 's
window is selected but her button on tasklist is pushed and active_task in the
tasklist doesn't point to it...

I think that's an old bug never found in tasklist but I can't find what is going
wrong.

Created attachment 49043 [details] [review]
same patch with b-m-p bugfix

I hope this patch fix b-m-p bug.

The problem was b-m-p (also xmms I think) has multiple windows (player,
playlist,...) but only one (the player) is registred as a task. So when b-m-p
receive the focus the callback « wnck_tasklist_active_window_changed » is
luched for each window composing b-m-p but only one has a corresponding task,
for others « wnck_tasklist_change_active_task » is lunched with a NULL
pointer and then _WnckTasklistPrivate::active_task become NULL witch is not
what we want !

I realy don't know if I fixed it the right way so please give me feedback :-)

Created attachment 49068 [details] [review]
syntax cleaning

Created attachment 49170 [details] [review]
better patch

 - use GList fonctions like «g_list_find» is better than recode it :-) So now
the code is far cleaner.

 - remove workaround for b-m-p bug, i'll open another bug for it since it has
nothing to do with scroll.

b-m-p 's bug is now on #310381

Created attachment 49286 [details] [review]
b-m-p bug doesn't matter now

now the b-m-p bug doesn't matter. I added «previously_active_task» witch is
the previous non-NULL task actived.

It works very well for me.

since bug #310381 seems to be fixed (by me) the last attached patch become
obsolete and the good one is "better patch".

Created attachment 49316 [details] [review]
same patch but against CVS

I don't think this is usable without bug 310381 being fixed first (if the user
has any transients not shown in the tasklist, continuing to scroll restarts at
one of the ends of the tasklist).  Note that it will be important to handle
group modal windows as well in that bug or this patch will introduce a usability
problem for that case.  So I'll mark this bug as depending on 310381.

I haven't tested this with grouped windows, but I have a feeling that users with
that setting wouldn't like this behavior at all.  But then again, punishing
users who use stupid settings (*grumble*) may not be such a bad idea after
all...  ;-)

The patch in 130308 that introduced scrolling in the workspace switcher does not
wrap when you hit the end of the list.  If this patch is to be accepted, it
shouldn't do so either.

The patch kind of sucks in mouse focus because it *always* starts at one of the
ends of the list (because any window is deactivated by moving the mouse to the
taskbar).  There's a related case in sloppy and click focus modes where if a
modal dialog is popped up but doesn't get focus and is modal to the window that
last had focus then all windows are defocused.  I believe in these cases, the
patch should start the scrolling from the most recently active task.  There's a
wnck_window_is_most_recently_activated() function that may help with this.

I'm still not sure with libwnck or wnck-applet should be patched; Mark patched
the latter in bug 130308 and so it seems to me that perhaps that's the
appropriate place for a patch like this but I don't know as I'm not a panel
maintainer.  I'd like Mark or Vincent to comment on this aspect.

Of course, there's the whole question of whether we want this behavior.  It does
add a consistency with the workspace switcher (though some, like Havoc, didn't
like the idea there either), but there's also some differences: (1) the user has
to be over the window list meaning that the mouse will be over one of the tasks
which will therefore be highlighted--but while scrolling through the windows
other tasks become highlighted.  So we kind of have this dual highlighting thing
going on that seems weird to me.  (2) Users have much more difficulty
associating an order with windows than they do with workspaces (We have a number
of bugs about this), and (3) scrolling through grouped tasks (as mentioned
above) doesn't seem to make sense.

answers for each paragraphs:

1) yes

2) it works but it's not usefull because it doesn't scroll in the same order
than what we see in the panel... i'll see if it's possible to take care of the
group order.

3) my patch doesn't warp neither ;-)

4) that is easy to solve with wnck_screen_get_previously_active_window() I
think... i'll take a look at that

5) Vincent said me that he doesn't know if it's possible to do that thing in
gnome-pannel...

6) Since it's possible for workspace I always try it with tasks, it's intuitive
for me, but maybe not for anyone else ? (1) there is 2 different highlight:
simply color change when mouse is over the button and when the task is active
the button is pushed down. I'm using this patch since i've wrote it and never I
got confused... (2) it's simply easyer to switch to next task by scrolling. but
ok, it's not THE KILLER FEACTURE.

3) Oh, right--I just had one of those windows that triggers bug 310381 near each
edge of my tasklist so I probably confused the wraparound thing.  :)

5) Ah, ok.

6) Well, I won't block it.  Just thought I'd mention it because any patch for a
feature like this needs to be approved by at least one other maintainer and if
we've covered the bases on issues it makes their job easier.  I agree that (1)
and (2) are minor issues and I'm not sure what I think about (3) but you're
looking into it anyway.

Created attachment 49577 [details] [review]
follow the on-screen order and takes care of groups

Ok so here I create «tasklist::windows_sorted» list, witch is the same as
«tasklist::windows» but with respect of the on-panel order (groups before and
then ungrouped tasks). Then when scrolling it is very simple, just get the next
or prev element in the list.

is it ok ?

Created attachment 49716 [details] [review]
maybe better ?

Don't build a second list of windows, but replace tasklist::windows by the
sorted one.

Comment on attachment 49716 [details] [review]
maybe better ?

Just some random comments. Don't take this as a full review :-)

>+      /* Build sorted windows list */
>+      if (g_list_length(task->windows) > 1)
>+        windows_sorted = g_list_concat(windows_sorted,
>+                                       g_list_copy(task->windows));
>+      else
>+        windows_sorted = g_list_append(windows_sorted, task);
>+

You need to add some spaces here, e.g.:
if (g_list_length (task->windows) > 1)

>+static gboolean
>+wnck_tasklist_scroll_cb (WnckTasklist *tasklist,
>+                         GdkEventScroll *event,
>+                         gpointer user_data)
>+{
>+  GList *window;
>+
>+  window = g_list_find(tasklist->priv->windows,
>+                       tasklist->priv->active_task);
>+    
>+  switch (event->direction)
>+    {
>+      case GDK_SCROLL_UP:
>+      case GDK_SCROLL_RIGHT:
>+        if (!window)
>+          window = g_list_last(tasklist->priv->windows);
>+        else
>+          window = window->prev;
>+      break;
>+      
>+      case GDK_SCROLL_DOWN:
>+      case GDK_SCROLL_LEFT:
>+        if (!window)
>+          window = tasklist->priv->windows;
>+        else
>+          window = window->next;
>+      break;
>+    }

Does it do the right thing if I have a tasklist with 4 lines and 3 columns? I'd
expect the left scroll to go left and the up scroll to go up :-)

How can I know howmany columns there are ? I think it's impossible (or very
difficult).

Created attachment 50944 [details] [review]
works scroll right/left change colomn, up/down change row

ok, this patch do its job.

add tasklist::n_rows. Scroll right/left switch the colomn.

I wanted this feature, and somewhat foolishly cooked up my own patch BEFORE
searching bugzilla.

For the record, I've attached my patch, which is for gnome-panel
(applets/wncklet) rather than libwnck (since the existing
scroll-changes-workspace behavior is in gnome-panel rather than libwnck,
although libwnck is probably more appropriate).  This does mean that it uses the
libwnck public API rather than its internals.

Also, it patches the window selector menu AS WELL AS the window list.  I've got
the menu in one of Fitt's corners which makes it easy to switch active windows
using only the mouse.

Now that we are past 2.12's feature freeze - what would it take to get accepted?

Created attachment 53780 [details] [review]
gnome-panel/applets/wncklet patch

Created attachment 54024 [details] [review]
yet another patch

I think your patch doesn't take care of grouped tasks and doesn't seperate
up/left, down/right.

My last patch was buggy with right/left if there is grouped windows. After
testings I think every goes fine with this attached patch.

Created attachment 54974 [details] [review]
getting better and better

Damn previous patch wasn't yet perfect. This last patch fixes a segfault
(startup tasks shouldn't be in windows list) and a bug when scrolling too fast.


Any comments ?

Created attachment 54978 [details] [review]
revised by vuntz

Same thing but with modifications recommended by vuntz.

Created attachment 54979 [details] [review]
better comments

Patch looks okay to me, but I'd love to see a small comment from Elijah, Havoc
or Mark :-)

Heh, so, after writing a test program it turns out that since Metacity sucks and
doesn't support group modal windows that libwnck's similar sucking doesn't
happen to hurt this bug.  Bug 310381 is already being left open for the
libwnck's lack-of-group-modal-support (and it needs to be fixed when Metacity
and gtk+ are fixed anyway), so that drops that objection I had to the patch.

So, since Vincent is okay with it, go ahead and commit.

2005-12-04  Vincent Untz  <vuntz@gnome.org>

        Add scrolling support to the tasklist. Patch by Xavier Claessens
        <x_claessens@skynet.be>. Fix bug #309956.

        * libwnck/tasklist.c: (wnck_tasklist_size_allocate): recreate the list
        of windows here, but have it sorted
        (wnck_tasklist_scroll_cb): new
        (wnck_tasklist_new): connect to scroll-event
        (wnck_task_compare): add a comment here about the fact that the
        windows sort used for scrolling depends on the position of the
        sequences in the sort