After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 166683 - Nicer sidebar widget
Nicer sidebar widget
Status: RESOLVED FIXED
Product: evince
Classification: Core
Component: general
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: Evince Maintainers
Evince Maintainers
Depends on:
Blocks:
 
 
Reported: 2005-02-08 16:01 UTC by Marco Pesenti Gritti
Modified: 2005-02-28 20:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch (14.64 KB, patch)
2005-02-16 21:53 UTC, Carlos Garcia Campos
none Details | Review
An updated patch (14.76 KB, patch)
2005-02-17 19:13 UTC, Carlos Garcia Campos
none Details | Review
The screenshot (32.19 KB, image/png)
2005-02-17 19:17 UTC, Carlos Garcia Campos
  Details
Another updated patch (15.09 KB, patch)
2005-02-18 17:31 UTC, Carlos Garcia Campos
none Details | Review
A new patch (25.19 KB, patch)
2005-02-23 12:19 UTC, Carlos Garcia Campos
none Details | Review
A patch for sidebar size persiting (5.18 KB, patch)
2005-02-23 17:32 UTC, Carlos Garcia Campos
none Details | Review
Evince sidebar mockup (8.17 KB, image/png)
2005-02-27 12:43 UTC, Teppo Turtiainen
  Details
A patch that fixes it (1.44 KB, patch)
2005-02-27 16:53 UTC, Carlos Garcia Campos
committed Details | Review
New screenshot (15.00 KB, image/png)
2005-02-27 16:55 UTC, Carlos Garcia Campos
  Details

Description Marco Pesenti Gritti 2005-02-08 16:01:43 UTC
The combobox looks somewhat ugly. I think something like nautilus would be
better... A close button would be useful too.

Bryan, sounds good?
Comment 1 Bryan W Clark 2005-02-08 19:08:54 UTC
Yeah, that sounds a lot better.  good idea
Comment 2 Carlos Garcia Campos 2005-02-16 21:53:51 UTC
Created attachment 37574 [details] [review]
Proposed patch

Hi again, 

Here is a patch for improving the sidebar :-)
Comment 3 Marco Pesenti Gritti 2005-02-17 19:03:57 UTC
Can you please upload a screenshot? So we see if there are visual aspects to
tweak... (Thanks very much for the patch btw)
Comment 4 Carlos Garcia Campos 2005-02-17 19:13:59 UTC
Created attachment 37612 [details] [review]
An updated patch

I found a small bug in the patch, here is an updated version.
Comment 5 Carlos Garcia Campos 2005-02-17 19:17:47 UTC
Created attachment 37613 [details]
The screenshot
Comment 6 Bryan W Clark 2005-02-17 20:14:43 UTC
Looks nice, I noticed in the earlier patch that the sidebar resized when I
changed from Thumbnail to Index, is that fixed in this version?
Comment 7 Carlos Garcia Campos 2005-02-18 17:31:58 UTC
Created attachment 37655 [details] [review]
Another updated patch

No :-P it's fixed now in this latest version
Comment 8 Marco Pesenti Gritti 2005-02-18 18:33:28 UTC
+	GtkWidget *label;
+	   
 
 	GtkTreeModel *page_model;

One \n here is enough

+static gboolean ev_sidebar_select_button_press_cb     (GtkWidget *widget,
+													   GdkEventButton *event,
+													   gpointer user_data);

I'd prefer to avoid all these declarations at the top by placing the static
functions just before the functions where they are used. Sorry, it's really just
a stupid nitpick... be patient ;)

+					GTK_TYPE_WIDGET,

Are you using tabs instead of spaces here? /me wonder why he did start yet
another project using tabs instead of spaces :(

+	hbox = gtk_hbox_new (FALSE, 0);
+	ev_sidebar->priv->hbox = hbox;
+	gtk_widget_show (hbox);
+	gtk_container_add (GTK_CONTAINER (frame), hbox);

Can you please show the widget after it's packed into another? There are several
cases after this. Not sure if it's just a style thing or if it has perf reasons
but I'm used to see it that way and it helps when scanning code.

+	close_button = gtk_button_new ();
+	gtk_button_set_relief (GTK_BUTTON (close_button), GTK_RELIEF_NONE);
+	g_signal_connect (close_button, "clicked",
+					  G_CALLBACK (ev_sidebar_close_clicked_cb),
+					  ev_sidebar);
+
+	gtk_widget_show (close_button);

Please remove the \n before _show, it's part of the same group

+	image = gtk_image_new_from_stock (GTK_STOCK_CLOSE,
+									  GTK_ICON_SIZE_SMALL_TOOLBAR);

Spaces facked up here?

+	gtk_menu_attach_to_widget (GTK_MENU (ev_sidebar->priv->menu),
+							   GTK_WIDGET (ev_sidebar),
+							   ev_sidebar_menu_detach_cb);

Here too?

+	
 	gtk_widget_show_all (GTK_WIDGET (ev_sidebar));
 }

This one can go, since you are already showing widget one by one. Though please
make sure you didnt forget some.

+static void
+ev_sidebar_size_allocate (GtkWidget *widget,
+					 GtkAllocation *allocation)

Spaces fucked up?

What is the reason of that size allocate functions. What in default allocation
needs to be corrected?

+static void
+ev_sidebar_menu_position_under (GtkMenu *menu,
+								int *x,
+								int *y,
+								gboolean *push_in,
+								gpointer user_data)

Spaces prolly. There are more of these. Please check what's the problem and fix
them.

+	if ((event->type == GDK_BUTTON_PRESS) && event->button == 1) {

It doesnt seem to be necessary to check event type, since it's a button_press event.

+	signals[CLOSE_REQUESTED] = g_signal_new
+		("close_requested",
+		 G_TYPE_FROM_CLASS (ev_sidebar_class),
+		 G_SIGNAL_RUN_LAST,
+		 G_STRUCT_OFFSET (EvSidebarClass, close_requested),
+		 NULL, NULL,
+		 g_cclosure_marshal_VOID__VOID,
+		 G_TYPE_NONE, 0);

I think in evince we can avoid the whole close_requested business. It should be
enough to connect to the visibility signal of the sidebar widget and update
chrome accordingly.

+static void
+ev_window_sidebar_size_allocate_cb (GtkWidget *widget, GtkAllocation *allocation,
+				    EvWindow *ev_window)
+{
+	gtk_paned_set_position (GTK_PANED (ev_window->priv->hpaned), allocation->width);
+}

I'm unclear on this too. This is probably coupled with the size allocation
function in the sidebar. /me hope there is a better way to do this but it's hard
to figure out without knowing the problem we are trying to solve.

As usual, if I'm unclear or you have doubts please ask. Thanks!
Comment 9 Carlos Garcia Campos 2005-02-18 19:37:08 UTC
> Are you using tabs instead of spaces here? /me wonder why he did start yet
> another project using tabs instead of spaces :(

No, I think the problem is my .emacs, it's configured to be compliant with the
GNOME code style guide, so it uses 8 spaces instead of 4.

> What is the reason of that size allocate functions. What in default allocation
> needs to be corrected?

without this size allocate function, when you move the paned to be smaller than
the sidebar, the close button is over the toggle button. Surely my explication
is not enough clear, so here is a screenshot. 

http://carlosgc.linups.org/files/evince.png

> +static void
> +ev_window_sidebar_size_allocate_cb (GtkWidget *widget, GtkAllocation
>*allocation,
>				    EvWindow *ev_window)
>{
>	gtk_paned_set_position (GTK_PANED (ev_window->priv->hpaned), >allocation->width);
>}
>
>I'm unclear on this too.

This function fixes what bryan says in the sixth comment.
Comment 10 Marco Pesenti Gritti 2005-02-18 20:43:47 UTC
>This function fixes what bryan says in the sixth comment.

I'm not clear of what Bryan was seeing and what's the wanted behavior... I'm not
sure what he means with "the sidebar was resizing". If the sidebar is the pane
here, than I think this is what your size_allocate_cb cause, not what it fixes?
But that would be when switching from a shorter label to a bigger, not the opposite.

/me is confused 
Comment 11 Bryan W Clark 2005-02-19 18:19:07 UTC
Oh, sorry about the confusion. 

What I saw in the earlier patch was that the sidebar was size X when I had
opened up a PDF and it was showing the Thumbnails.  When I switched to Index the
sidebar shrunk, I believe because the string is shorter.  Of course we don't
want the sidebar to shrink like that when you select another item.  

I'll try to check out this patch tomorrow to see if it's all fixed.
Comment 12 Marco Pesenti Gritti 2005-02-20 01:10:11 UTC
I think I know the reason of that behavior. If there is not an user set
position, gtk base the paned position on widgets allocation.
We need to persist the sidebar position anyway, so I'd say to ignore the issue
in this bug. When we implement sidebar position, we can set to a default value
the first time, that will solve this issue too.

Carlos, can you please remove ev_window_sidebar_size_allocate_cb when submitting
an updated patch?
Or feel free to implement persist of the paned position ;) I think we just need
to save the size to gconf on window destroy signal, and to load it from gconf on
window creation (fallback to a default size if there is not one in gconf).
Comment 13 Carlos Garcia Campos 2005-02-20 09:54:57 UTC
> I think I know the reason of that behavior. If there is not an user set
> position, gtk base the paned position on widgets allocation.
> We need to persist the sidebar position anyway, so I'd say to ignore the issue
> in this bug. When we implement sidebar position, we can set to a default value
> the first time, that will solve this issue too.

Yes, this is the reason. 

> Carlos, can you please remove ev_window_sidebar_size_allocate_cb when submitting
> an updated patch?

Ok.

> Or feel free to implement persist of the paned position ;) I think we just need
> to save the size to gconf on window destroy signal, and to load it from gconf on
> window creation (fallback to a default size if there is not one in gconf).

Yeah! I was thinking in fixing #164811 too, because it's directly related.
Comment 14 Jonathan Blandford 2005-02-23 04:41:12 UTC
I tried the sidebar patch tonight.  It looks quite good, though I think that
shrinking the sidebar less than the buttons is a bit weird.  We might want to
either keep that the minimum size, or make the label in the button ellipsize.  I
think that we should get this patch in, and file new bugs for any other issues
we run into.
Comment 15 Marco Pesenti Gritti 2005-02-23 09:14:47 UTC
I think ellipsizing is probably the best solution here. It was just not
available at the time the original sibebar widget was written. (Should probably
patch epiphany and nautilus too).

Carlos, do you have time to remove the two size_allocate, fix the nitpicks and
check in? I'd like to get this in the next release. We can fix the problems later...
Comment 16 Carlos Garcia Campos 2005-02-23 09:39:55 UTC
Yes, I was preparing a new patch but I have a problem. You said that we can use
the visible signal of the sidebar instead of its own close_requested. The
problem is that update_chrome_flag calls to update_visibility, so the visible
property is changed and the notify signal is emitted again in an infinite loop.
I've tried blocking the signal, but it doesn't work for me (I don't know why). 
Comment 17 Marco Pesenti Gritti 2005-02-23 09:52:53 UTC
In update_chrome_visibility:

g_object_set (priv->sidebar, "visible", sidebar, NULL);

If you use gtk_widget_show/hide instead of this, it's a bit more code but it
should avoid the infinite cycle.
Comment 18 Carlos Garcia Campos 2005-02-23 12:19:30 UTC
Created attachment 37828 [details] [review]
A new patch

I think the problem with the spaces is not fixed in the patch, but I don't know
how to fix it. I remove them with emacs, but when I do cvs diff the spaces
appear again.
Comment 19 Marco Pesenti Gritti 2005-02-23 13:47:44 UTC
Reformatted this and checked in. I stripped out the size persisting thing for
now, I'd like to review that in a separate patch.
Comment 20 Carlos Garcia Campos 2005-02-23 17:32:20 UTC
Created attachment 37846 [details] [review]
A patch for sidebar size persiting

This patch also fixes an issue of the previous patch. I've only moved the
connection of the sidebar visibility signal after the creation of the other
widgets. It avoids calling to gtk_widget_show for widgets not created yet.
Comment 21 Teppo Turtiainen 2005-02-27 12:42:40 UTC
The new sidebar widget in much nicer, but still not perfect. The combobox should
not change size according to the selection (Index or Thumbnails) but rather
always be as wide as the sidebar is so there is no empty space between the
combobox and the close button. See attached mockup.
Comment 22 Teppo Turtiainen 2005-02-27 12:43:43 UTC
Created attachment 38000 [details]
Evince sidebar mockup
Comment 23 Carlos Garcia Campos 2005-02-27 16:53:48 UTC
Created attachment 38006 [details] [review]
A patch that fixes it

I think it's possible to obtain that behaviour with the current sidebar.
Comment 24 Carlos Garcia Campos 2005-02-27 16:55:29 UTC
Created attachment 38007 [details]
New screenshot
Comment 25 Marco Pesenti Gritti 2005-02-28 09:01:53 UTC
Comment on attachment 38006 [details] [review]
A patch that fixes it

Sounds like a good idea
Comment 26 Carlos Garcia Campos 2005-02-28 14:48:20 UTC
So, is it ok to commit?
Comment 27 Marco Pesenti Gritti 2005-02-28 19:55:31 UTC
Sure, see patch status there is a "accepted-commit_now" :)
Comment 28 Carlos Garcia Campos 2005-02-28 20:10:43 UTC
ups, sorry, I didn't see it. It's committed now :-)