Created attachment 20813 [details] [review]
Proposed patch

Oh, I forgot to say that I added a way to specify the maximum width of
the buttons. I'll make a patch for the workspace switcher to use this
parameter, if it sounds like a good idea.

Can you explain the patch a bit? i.e. what causes the bug and why does
the patch fix it.

Specifying max button size seems yucky to me if the user has to do it
manually, but if you somehow based it on number of windows, or panel
size, that might be nice.

I should have elaborated a bit on what the patch does, yes.
Here we go:
 
In wnck_tasklist_size_request(), we look if a mandatory height or
width was set using gtk_widget_get_size_request() (I did this because
this is what the window-list applet does). And if this is the case, we
use this mandatory parameter and try to request a size conforming to it.
 
We try some layouts depending on this mandatory parameter and on the
grouping option: if there is a mandatory width, then we know that we
won't be able to have more than n_cols. Else, we'll just have one
column (this might be fine-tuned, e.g. we may just want to have just
one row if there is no mandatory height either).

The old behaviour of wnck_tasklist_size_request() looks broken to me:
the widget always set the GtkRequistion to the minimum size of the
widget, but we don't always need this size. The behaviour after my
patch requests only what is needed. Moreover, it tried to do some
layouts with a fake allocation that is far from the allocation that
will be given.

As for the change in wnck_tasklist_size_allocate(): the patch makes
the button always the same height. Right now, if you have one window
on a large panel, its button will occupy all the height of the panel
and when you open a second window, the button's height changes. With
the path, the height does not change. Moreover I made sure that every
allocated pixel is used by a button when we have a full column of
buttons (we don't break Fitt's law).

I think that's it. I'm not sure I was clear and I may have forgotten
one or two things. But I need to sleep...

cc'ing Alex on this too.

I had a quick look at this.

Having a prefs for maximumg button size sounds totally wrong. We
totally don't want to get back the gnome 1.4 "user has to enter set of
nonobvious pixel values" dialog. 

I'm not quite sure what you mean with the size request requesting only
what is needed. If we have a minimum size, surely we need at least
that size?

+/* set the mandatory maximum button width
+ * use -1 to unset it */
+void 
+wnck_tasklist_set_mandatory_max_button_width (WnckTasklist *tasklist,
gint size)
+{
+  g_return_if_fail (WNCK_IS_TASKLIST (tasklist));
+
+  if (size == -1) size = DEFAULT_HEIGHT;
+

This looks wrong. Surely unsetting should set it to -1, not 48?

+      /* We want to use as many rows as possible to limit the width */
+      n_cols = n_buttons / n_rows;
+      if (n_buttons % n_rows > 0)
+        n_cols++;
+

and:

   /* We want to use as many rows as possible to limit the width */
-  n_cols = (n_buttons + n_rows - 1) / n_rows;
+  n_cols = n_buttons / n_rows;
+  if (n_buttons % n_rows > 0)
+    n_cols++;

Why this change? This calculates the same thing, but using two
divisions instead of one.

Also, wnck_tasklist_layout and wnck_tasklist_try_layout seems
very similar, and have duplicated code. Can't the code be shared better?

+      if (req_width == -1)
+        requisition->width = MAX (tasklist->priv->minimum_width -
n_cols * tasklist->priv->max_button_width, 0);
+      if (req_height == -1)
+        requisition->height = MAX (tasklist->priv->minimum_height -
n_rows * tasklist->priv->max_button_height, 0);

I don't get this? We request less than the minimum size? And with a
lot of buttons we always request 0 pixels?

+    if (!score_set && tasklist->priv->grouping !=
WNCK_TASKLIST_NEVER_GROUP)
+      {
+        wnck_tasklist_score_groups (tasklist, ungrouped_apps);
+        score_set = TRUE;
+      }
+
   while (ungrouped_apps != NULL &&
 	 tasklist->priv->grouping != WNCK_TASKLIST_NEVER_GROUP)
     {
-      if (!score_set)
-	{
-	  wnck_tasklist_score_groups (tasklist, ungrouped_apps);
-	  score_set = TRUE;
-	}
 
Why did you move this code? (Same in another place)
It just seems to run score_groups unnecessary if there are no
ungrouped apps?

-      int col = i % n_cols;
-      int row = i / n_cols;
+      int col = i / n_rows;
+      int row = i % n_rows;
       
This looks wrong. 

> Having a prefs for maximumg button size sounds totally wrong. We
> totally don't want to get back the gnome 1.4 "user has to enter set of
> nonobvious pixel values" dialog.

I added this because I think it might be useful (there's at least one
bug requesting that). And we have right now these nonobvious pixel
values in the window list prefs with minimum and maximum sizes of the
window list. But that's not the goal of the patch, so let's remove
this part of the code.

> I'm not quite sure what you mean with the size request requesting only
> what is needed. If we have a minimum size, surely we need at least
> that size?

The size_request function is doing two things: it does a size request
for the tasklist widget and it calculates a size_hints. As far as I
understand the code, the size request does not count the buttons: the
requistion is always the length of the minimum size. Size of buttons
is given to the parent in the size_hints.

But if we have a button that is larger than this minimum size, we
don't need to request this minimum because thanks to the size_hints,
we will have a bigger size (mmhhh... maybe this assertion is false?).
This change does need a change in the window list applet. Something like:

@@ -558,7 +558,7 @@
         * where min(i) > max (i+1)
         * convert it to clipped values
         */
-       maximum_size = tasklist->maximum_size - minimum_size;
+       maximum_size = tasklist->maximum_size;
        g_assert (maximum_size >= 0);
  
        for (i = 0; i < len; i += 2) {

But if I'm wrong, let's just keep this as it is.

> +/* set the mandatory maximum button width
> + * use -1 to unset it */
> +void 
> +wnck_tasklist_set_mandatory_max_button_width (WnckTasklist *tasklist,
> gint size)
> +{
> +  g_return_if_fail (WNCK_IS_TASKLIST (tasklist));
> +
> +  if (size == -1) size = DEFAULT_HEIGHT;
> +
> 
> This looks wrong. Surely unsetting should set it to -1, not 48?

Yep, you're right.

> +      /* We want to use as many rows as possible to limit the width */
> +      n_cols = n_buttons / n_rows;
> +      if (n_buttons % n_rows > 0)
> +        n_cols++;
> +
> 
> and:
> 
>    /* We want to use as many rows as possible to limit the width */
> -  n_cols = (n_buttons + n_rows - 1) / n_rows;
> +  n_cols = n_buttons / n_rows;
> +  if (n_buttons % n_rows > 0)
> +    n_cols++;
> 
> Why this change? This calculates the same thing, but using two
> divisions instead of one.

I did this because I find it more readable, but it's fine for me to
keep the old way to do it.

> Also, wnck_tasklist_layout and wnck_tasklist_try_layout seems
> very similar, and have duplicated code. Can't the code be shared better?

It can be shared, but I thought it was better to have two functions
because they don't do the same thing. But I can do this if you want.

> +      if (req_width == -1)
> +        requisition->width = MAX (tasklist->priv->minimum_width -
> n_cols * tasklist->priv->max_button_width, 0);
> +      if (req_height == -1)
> +        requisition->height = MAX (tasklist->priv->minimum_height -
> n_rows * tasklist->priv->max_button_height, 0);
> 
> I don't get this? We request less than the minimum size? And with a
> lot of buttons we always request 0 pixels?

See my previous answer about the size_hints.

> +    if (!score_set && tasklist->priv->grouping !=
> WNCK_TASKLIST_NEVER_GROUP)
> +      {
> +        wnck_tasklist_score_groups (tasklist, ungrouped_apps);
> +        score_set = TRUE;
> +      }
> +
>    while (ungrouped_apps != NULL &&
>  	 tasklist->priv->grouping != WNCK_TASKLIST_NEVER_GROUP)
>      {
> -      if (!score_set)
> -	{
> -	  wnck_tasklist_score_groups (tasklist, ungrouped_apps);
> -	  score_set = TRUE;
> -	}
>  
> Why did you move this code? (Same in another place)
> It just seems to run score_groups unnecessary if there are no
> ungrouped apps?

In my mind, it's better to not test every time in a loop, but just
test one time before the loop. We can add a test on ungrouped_apps !=
NULL or I can change it back.

> -      int col = i % n_cols;
> -      int row = i / n_cols;
> +      int col = i / n_rows;
> +      int row = i % n_rows;
>        
> This looks wrong.

If you have a big panel, with the old way, the tasklist got filled one
row after another. But the layout first calculates how many row we can
have, and then the number of columns. So we can have this:

======================================
First application | Second application
Third Application | Fourth application
(Nothing)         | (Nothing)
======================================

I do not feel having a void row is really good.

Now we have:

=======================================
First application  | Fourth application
Second application | (Nothing)
Third application  | (Nothing)
=======================================

To keep the old ordering, we could just change the height of the
button, but it won't look good if a new window is open: the buttons
will all resize. With the new behaviour, it does feel good. Well, IMHO.

> I added this because I think it might be useful (there's at least one
> bug requesting that). And we have right now these nonobvious pixel
> values in the window list prefs with minimum and maximum sizes of the
> window list. But that's not the goal of the patch, so let's remove
> this part of the code.

I'm sure people have requested all sorts of prefs. Thats what people
tend to do when they find something that isn't quite right. They think
it was designed to work that way, so they request a preference so that
they can manually fix it to work for them. They seldom sit down and
think about all uses of the app and propose a redesign that makes it
work automatically for everyone.

Its unfortunate that we have the max/min size prefs, but avoiding
these was very hard, and at least they are very direct and obvious
settings.

> ... size hints, etc ...

The size hints are just a hint, aren't they? I though the reason the
size_request was set to the minimum was to make sure the applet never
ever got less than the minimum size.

Also, theoretically the wnck widget is a normal widget usable outside
the applet, so we shouldn't make its size behaviour depend on
panel-only features.

>> Why this change? This calculates the same thing, but using two
>> divisions instead of one.
>
>I did this because I find it more readable, but it's fine for me to
>keep the old way to do it.

It doesn't matter much to me either, but i find the code with a
control structure (the if) harder to read.

>It can be shared, but I thought it was better to have two functions
>because they don't do the same thing. But I can do this if you want.

I didn't look that closely, but it looked as if one did the same as
the other and then some, so that it could possibly call that, but its
not a huge thing.

>In my mind, it's better to not test every time in a loop, but just
>test one time before the loop. We can add a test on ungrouped_apps !=
>NULL or I can change it back.

Oh, its probably ok. I just wondered if I missed some reason it had to
be done or something.

> =======================================
> First application  | Fourth application
> Second application | (Nothing)
> Third application  | (Nothing)
> =======================================

This isn't the normal order people read in though
(typically left-to-right-top-to-bottom), and there is no hint that the
ordering is unusual, so I think this will just make the tasklist order
seem random to people.

Created attachment 20997 [details] [review]
Updated patch.

I updated the patch. Here are the changes:
     * there's no more the max button size pref
     * I reversed the change to the request/size_hints thing so we
always request the minimum size (I'll add a comment in the window-list
applet code to explain why we substract minimum_size)
     * I reversed the change to the way we find how many columns we'll
have
     * I simplified wnck_tasklist_try_layout

But I did not change the way we order the button because I really
don't like the ideas of having a void row or seeing the height of the
buttons change when you add a windows. I understand your concern about
the order of the buttons, though. Right now, buttons are ordered by
xid, so I'm just not sure the order is really useful. Or does anyone
have a better proposition to solve this problem?

There's a bug in the patch: it should be

  requisition->width = MAX(req_width, tasklist->priv->minimum_width);
  requisition->height = MAX(req_height, tasklist->priv->minimum_height);

instead of

   requisition->width = MAX(requisition->width,
tasklist->priv->minimum_width);
   requisition->height = MAX(requisition->height,
tasklist->priv->minimum_height);

Because we don't always know what's in requisition.

And there's another problem: we provide a size_hints for the width,
but not for the height, so when you have a vertical window-list
applet, we use a bad size_hints in the applet. There's a similar
problem for the grouping_limit, I suppose. It needs some investigation...

AFAIK, size hints hint at height, if panel is vertical.
What about rotating buttons 90 CCW if we have a vertical panel, BTW?
There is no vertical text label in gtk+ :(

*** Bug 126532 has been marked as a duplicate of this bug. ***

I need to update the patch for 2.7.
Alex: will you have some time to look at it now that 2.6.0 is out?

Alex? Comment on Vincent's question?

*** Bug 152752 has been marked as a duplicate of this bug. ***

See also bug 155870.

*** Bug 158264 has been marked as a duplicate of this bug. ***

So, this bugreport has been open for more than a year. By the look of it, fixes
have been proposed. Is there ANY chance that it will be fixed within my
life-span, asuming I will reach the age of 80?

Richard: Your comments aren't helpful.  Please read
http://bugzilla.mozilla.org/page.cgi?id=etiquette.html (which also applies to
Gnome bugzilla).  I know it's frustrating to have bugs that affect you remain
unfixed, but the best way to improve Gnome is to help the developers, or, if
that isn't possible, to avoid hindering them.  Thanks.

hello,

this issue still occurs. i tried it on fedora 3 desktop install. open firefox,
the size is normal. type a url and then the size of task bar entry expands to
more than normal. then aagin open another window and size is back to normal. So
this happens when : open firefox and type a url and press enter. that is one
instance this happens.

feodra 3 - gnome 2.8

Moving to the right component. Sorry for the spam.

Any chance seeing this fixed? There seems to be a patch for this...

Consolidating; see bug 310809.

*** This bug has been marked as a duplicate of 310809 ***