I agree

Yeah, sounds reasonable.  I guess it shouldn't be a combobox at all if there
isn't more than one selection.  I don't think that will hurt a11y

Thinking about this more, we should probably leave it as is in the combobox,
however disable the index element when it's not available.  Bug 164755 is
similar and that's the conclusion for that one.

+static void
+ev_window_dragged_file_cb (GtkWidget *widget, GdkDragContext *context,
+			   gint x, gint y, GtkSelectionData *selection_data,
+			   guint info, guint time, gpointer gdata)

Please call this drag_data_received_cb

+{
+	GList    *uris = NULL;
+	EvWindow *ev_window;
+	gchar    *mime_type;
+	gchar    *uri;
+
+	if (info == 0) {

What is the reason of this check?

+		uris = gnome_vfs_uri_list_parse ((gchar *) selection_data->data);
+
+		while (uris != NULL && uris->data != NULL) {

I dont think you ned to check uris->data here. You can just put a
g_return_if_fail inside the while, if you want to be safe.

+			uri = gnome_vfs_uri_to_string (uris->data,
+						       GNOME_VFS_URI_HIDE_NONE);
+			mime_type = gnome_vfs_get_mime_type (uri);
+
+			if (mime_type &&
+			    ((g_ascii_strcasecmp (mime_type, "application/pdf") == 0) ||
+			    (g_ascii_strcasecmp (mime_type, "application/postscript") == 0) ||
+			    (g_ascii_strcasecmp (mime_type, "image/x-eps") == 0))) {

I think we should not duplicate the mime checks here. Maybe the check should be
factored about to a gboolean is_file_supported func or similar and used in both
places.

+				if (ev_window_is_empty (EV_WINDOW (widget))) {
+					ev_window = EV_WINDOW (widget);
+				} else {
+					ev_window = ev_application_new_window (EV_APP);
+				}
+				
+				ev_window_open (ev_window,
+						gnome_vfs_uri_to_string (uris->data,
+									 GNOME_VFS_URI_HIDE_NONE));

We should probably have a ev_window_open_uri_list, static for now, and have this
code there.

+				gtk_drag_finish (context, TRUE, FALSE, time);
+				gtk_widget_show (GTK_WIDGET (ev_window));
+			} else {
+				gtk_drag_finish (context, FALSE, FALSE, time);
+			}

I dont think we need to call drag_finish here.

+			if (mime_type) g_free (mime_type);
+			if (uri) g_free (uri);

g_free is NULL safe no need for these checks.

+			uris = g_list_next (uris);
+		}
+			
+		gnome_vfs_uri_list_free (uris);

Arent you freeing a NULL here? You should use a GList *l to iterate. But please
see my previous comment about factoring out the code to open a list of uris in a
window.

+	}
+}

Wow I need sleep... this is obviously the wrong bug.

Thu Feb 24 23:07:33 2005  Jonathan Blandford  <jrb@redhat.com>

        * shell/ev-window.c (hide_sidebar_and_actions): Hide the sidebar
        iff the type doesn't support thumbnailing and indexing.

Another nice thing - should not display thumbnails with only 1 page.

Created attachment 45310 [details] [review]
Patch to CVS

Patch commited

Suggestions from jrb, since he doesn't like recent state

(22:17:54) nsh: So jrb, about sidebar - there should be sidebar widget and
side-bar page interface and some objects that implements EvSidebarPage
(22:18:09) jrb: nsh: I think so
(22:18:22) jrb: nsh: though I can only think of one more Sidebar to add
(22:18:23) nsh: EvSidebarPage should have the following methods -
_support_document and set_document
(22:19:31) nsh: After start EvSidebar should check all available sidebar pages
if they can support current document and add only those pages to sidebar notebook
(22:20:15) nsh: If there is no pages in notebook it should be hidden but still
available for user with menu or f9 button
(22:24:15) nsh: If all this looks ok, I'll update bug 164811 and will try to
implement this. 
(22:25:45) jrb: nsh: I think so
(22:25:47) jrb: nsh: also
(22:25:54) jrb: maybe make a superclass
(22:25:55) jrb: dunno
(22:26:04) jrb: or an interface, maybe

Created attachment 45337 [details]
Tar.gz with patch and sources

The start of work on implementing sidebar pages as objects with EvSidePage
interface. 

The problem now is how to handle EvSidebarLinks model in that scheme. It's
property that is exposed from EvSidebarLinks to EvPageAction. Such thing makes
interface extraction harder.

Created attachment 45352 [details]
Updated tar.gz with patch

Created attachment 45355 [details]
One more update

Patch looks fine; please commit.

In the future, though, can you 'cvs add' the files and just do a cvs diff --
it's easier to deal with a patch with the files added that way than the tarball,
as I can read the patch in bugzilla.

Path applied