Thanks for the bug report. This particular bug has already been reported into our bug tracking system, but please feel free to report any further bugs you find.


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

Oops, wrong duplicate sorry :)

Perhaps the ideal solution is to check for existence before the user clicks "Open", and enable/disable the Open button accordingly.

A second-best solution would be to leave the "Open Location" dialog open when the "Could not find <location>" dialog is shown so the user can correct the error.

I prefer the first solution because it doesn't use an alert dialog to tell the user he/she has made an error.

David, is the first option even possible - let's say I'm entering a ssh/ftp path that requires login information, either a dialog will popup asking for credentials or we have to take a chance that the server/path exists and we're back to square one.

I think hiding the "Open Location" dialog until the path has been shown and use a callback to close / show the dialog (and an error in the dialog, explaining why we're back) would be the best.

Created attachment 139577 [details] [review]
Displays open location dialog on failure opening the folder

This patch is one part of the larger patch at https://bugs.launchpad.net/hundredpapercuts/+bug/385785 that checks if the local path exists and if not, disables the Open button completely. Problem with that patch is it does blocking I/O.

A problem with the patch is that it only works in browser mode (why??) but if this is the right approach to the problem I'll fix that.

Comments?

I don't like the way this patch mixes up the specific open with dialog with the generic code. A more proper solution would be to add an optional callback to nautilus_window_go_to() that gives the resulting status, and then use that from the open with dialog.

Created attachment 143067 [details] [review]
Updated patch as wished by Alexander

Alexander, something like this?

Comment on attachment 143067 [details] [review]
Updated patch as wished by Alexander

Still needs work. 

Better off defining a type for the callback function (for lack of anything better at the moment, NautilusWindowGotoFunc), which makes all of the code look better. NautilusWindowGotoFunc should pass a const char *, if we really want that field at all; seems like we could do better here since the only reason we use it is when we're displaying errors. Why not pass the GError?

There are some other minor details: bad indentation in a few places, you're defining a variable mid-block in error_message_response_callback(), unnecessary cast in the same function, open_callback_userdata should just be a gpointer, etc.

Created attachment 143099 [details] [review]
Updated patch

Fixed most of the issues (unless I've missed something) in previous comment by awalton but still using a const char instead of GError because not all scenarios gets a GError (for example trying to "opening" a file).

Now that we're using git and stuff it would be cool to start using a
more git-like commit history with micro-commits. For instance, a
change like this is really a two commit thing, first change go_to() to
take a new argument, then add the required feature using this new API
in a second patch. Using git its very easy to do this (using topic
branches or just two regular commits), and using git-bz its easy to
push this to bugzilla.

Anyway, thats not real critisism, just a i-wish thing. Back to the
code:

I don't think passing the char *error makes much sense, especially
since we don't need it atm. Just pass in a gboolean success. If we
need more (like maybe what the resulting slot was) we'll add it then.

The use of open_callback in nautilus_window_go_to() is kinda
sloppy. We need to push these in as arguments all the way into
begin_location_change() so that we can handle it like the other
similar per-location-change data (such as e.g. pending_location) and
correctly call/free it in end_location_change or free_location_change
as needed. Otherwise things will go wrong if e.g. a second goto call
is done before the first one got called back, or if the window is
destroyed before its called back, etc.

+	g_signal_connect (dialog,
+			  "response",
+			  G_CALLBACK (error_message_response_callback),
+			  NULL);
+
+	g_object_set_data (G_OBJECT (dialog), 
+			   "cb_error", 
+			   error_message);
+	...

Just pass an allocatid struct as user data to the signal handler, no
need for this user data stuff.

Created attachment 145226 [details] [review]
Reopens the Open Location dialog after failure to open the entered location

Is the latest patch better?
Can we get this scheduled for 2.30 ?

With Nautilus 2.28.4 the open location dialog seems to have vanished. When I press Ctrl+L, the focus goes to the address bar. Personally I prefer it instead it of dialog.

That would make this bug redundant. Can anyone confirm?

@Sebastian, it's still there but in browser mode it works the way you described, by focusing the address bar. If you switch to spatial browsing or press Ctrl+L when the desktop has focus - then the dialog appears.

Also adding the patch review from Alexander;

I don't think you're correctly handling the case where we start loading
another location before the callback has been called, or if the window
is closed early. You need to handle this in end_location_change() if
there is a callback set.

alex, this is what's done in end_location_change(), isn't that enough?

	if (slot->open_callback) {
		/* callback has not been called - call it with success */
		slot->open_callback (NULL, TRUE, slot->open_callback_userdata);
	}
	slot->open_callback = NULL;
	slot->open_callback_userdata = NULL;

If so, I've got an updated patch for git master ready.

Created attachment 157836 [details] [review]
Updated patch against git master

Hmm.. strange thing I didn't notice earlier (or maybe it was working correctly then?); if I have an open nautilus window and the open location dialog reappears it will not have focus (the nautilus window will). I tried gtk_widget_grab_focus() and gtk_window_present() without success. Is this a problem of mine or the wm (in this case compiz)? It's working correctly otherwise.

Review of attachment 157836 [details] [review]:

Looks almost good.

As a general comment on the approach, I'd rather have separate functions for the callback versions of nautilus_window_slot_info_open_location() and nautilus_window_go_to(), otherwise we need to change all the other references to those functions around (as you do in your patch) making the code a bit less readable with all the NULLs.

::: eel/eel-stock-dialogs.c
@@ +397,3 @@
 
+	g_signal_connect_after (dialog, "response",
+			  G_CALLBACK (gtk_widget_destroy), NULL);

Why is this needed here?

::: src/nautilus-window-manage-views.c
@@ +1736,3 @@
+	if (slot->open_callback) {
+		/* callback has not been called - call it with success */
+		slot->open_callback (NULL, TRUE, slot->open_callback_userdata);

You should never pass NULL as the first argument of the callback, as callers might rely on that being a pointer to a NautilusWindow.

@@ +1880,3 @@
+
+	if (params->callback) {
+		params->callback (NULL, FALSE, params->userdata);

Ditto.

@@ +1984,3 @@
+	params = g_new (ErrorCallbackParameter, 1);
+	params->callback = window->details->active_pane->active_slot->open_callback;
+	params->userdata = window->details->active_pane->active_slot->open_callback_userdata;

It's better to use g_slice_new() and friends for this kind of small structure allocations.

Created attachment 165054 [details] [review]
Updated patch against 2.31.1

Cosimoc, thanks for reviewing.

About the _after change in eel; because I connect to a "response" event on the dialog and the eel event comes first it destroys the dialog and I won't get an event in display_view_selection_failure().

Good idea to get a new function for the callback functions, touches fewer files and easier to read. Thanks!

I'm not completely sure of how to use g_slice_new() and find using ErrorCallbackParameter makes the code more clear. But if you'd like me to change, I will :) (Note, in this patch I added "window" to the struct as well)

(In reply to comment #19)

> About the _after change in eel; because I connect to a "response" event on the
> dialog and the eel event comes first it destroys the dialog and I won't get an
> event in display_view_selection_failure().

Can't you connect to the 'destroyed' signal instead? You don't seem to use response_id anyway. I'd rather not change widely-used eel code.

> I'm not completely sure of how to use g_slice_new() and find using
> ErrorCallbackParameter makes the code more clear. But if you'd like me to
> change, I will :) (Note, in this patch I added "window" to the struct as well)

g_slice_new() and g_slice_free() are just replacements for g_new() and g_free(), using an optimized memory allocator different from malloc(), see [1]. Probably it's just a nitpick of mine, but it's quite common convention to use that family of functions for small volatile structs.

[1] http://library.gnome.org/devel/glib/unstable/glib-Memory-Slices.html

Created attachment 165230 [details] [review]
Final (?) patch to reopen open location dialog on error

@Cosimoc, "destroy" worked as well, thanks. I now also understand what g_slice_new() is and using that in this patch.

Review of attachment 165230 [details] [review]:

Marcus, thanks for the updated patch.
Most of the callback infrastructure that was required for this patch is now in place in Nautilus master, so this would just need to be reworked and updated to use it.

Similar to bug 494313

The "open location" dialog is gone now and behaviour for the location bar is already covered by bug 494313.