GNOME Bugzilla – Bug 166683
Nicer sidebar widget
Last modified: 2005-02-28 20:10: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?
Yeah, that sounds a lot better. good idea
Created attachment 37574 [details] [review] Proposed patch Hi again, Here is a patch for improving the sidebar :-)
Can you please upload a screenshot? So we see if there are visual aspects to tweak... (Thanks very much for the patch btw)
Created attachment 37612 [details] [review] An updated patch I found a small bug in the patch, here is an updated version.
Created attachment 37613 [details] The screenshot
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?
Created attachment 37655 [details] [review] Another updated patch No :-P it's fixed now in this latest version
+ 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!
> 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.
>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
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.
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).
> 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.
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.
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...
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).
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.
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.
Reformatted this and checked in. I stripped out the size persisting thing for now, I'd like to review that in a separate patch.
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.
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.
Created attachment 38000 [details] Evince sidebar mockup
Created attachment 38006 [details] [review] A patch that fixes it I think it's possible to obtain that behaviour with the current sidebar.
Created attachment 38007 [details] New screenshot
Comment on attachment 38006 [details] [review] A patch that fixes it Sounds like a good idea
So, is it ok to commit?
Sure, see patch status there is a "accepted-commit_now" :)
ups, sorry, I didn't see it. It's committed now :-)