417952 - Open Containing Folder doesn't highlight/select file in Nautilus

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[tester]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12
Build Identifier: Firefox 3 beta 3

if you go to tools>downloads>and then right click on "open containing folder", firefox will open up Nautilus.

the problem is it need to highlight the selected file (like on windows)

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Stephen Donner [:stephend] Not actively reading bugmail]

Which version of which Linux distribution are you using?

Comment 2

[tester]

Ubuntu 7.10 (i don't think it's relevant to this bug).

Comment 3

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #2)
> Ubuntu 7.10 (i don't think it's relevant to this bug).

I'm not sure why you think it's not, but regardless, I can confirm the bug.

Michael: any chance you could fix this for beta 4/final, since you fixed bug 391980?

Comment 4

[Tim Lauridsen]

I see this in Fedora 11 Preview (Gnome) too.

firefox-3.5-0.20.beta4.fc11.i586

When i click on 'Open Contain folder' i get a dialog asking what application to use.

Fedora downstream bug report.
https://bugzilla.redhat.com/show_bug.cgi?id=497710

Comment 5

[mFat]

This is a very annoying problem. Especially when your download folder contains a lot of files.

Comment 6

[rrae]

I played around modifying another filemanager (thunar), to see which parameters are given, when "Open Containing Folder" is clicked:

It only gets "/home/rrae/Downloads" as a parameter.

I think that's insuficcient. Whether or not file managers handle something like...

"/home/rrae/Downloads/somefile.tar.bz2"

... in a way as it is in Explorer is out of scope for this bug report, imho Firefox should do a first step in this direction.

Comment 7

[Shree Kant Bohra]

The bug in nautilus has been removed https://bugzilla.gnome.org/show_bug.cgi?id=632427 now it's firefox's turn to make the changes. Any taker for this?

Comment 8

[Matt Brubeck (:mbrubeck)]

Here's the code that constructs and opens the folder path:
https://hg.mozilla.org/mozilla-central/file/ae8de2241732/xpcom/io/nsLocalFileUnix.cpp#l1819

Note that it is not passed directly to Nautilus on the command line, but instead goes through gnome_vfs_url_show_with_env().  We can't just pass a filename to that function, because that will open the file itself in its app (rather than showing it in Nautilus).

However we fix this, we should also make sure that it does not break badly on old versions of Nautilus.

Comment 9

[Nicolas Barbulesco]

I encounter the problem, in Firefox 16.0.2, on Linux, Ubuntu, with Nautilus 2.32.2.1. It would be nicer to have the file selected.

Comment 10

[vulcain]

Reproduce with Firefox Nightly 21.0a1 (2013-02-13) on Ubuntu 12.04 with Nautilus 3.4.2
They will have a new Download Manager
https://wiki.mozilla.org/User:P.A./Panel-based_Download_Manager/Status

Someone knows if this behavior could be done with another browser with Nautilus or in KDE, XFCE with Firefox ??

Comment 11

[firefox]

The behaviour still persists. Is there some progress on this case?

Comment 12

[Nelson Benítez León]

Attachment: 0001-Bug-417952-Open-Containing-Folder-doesn-t-highlight-.patch

Hi, this patch uses the File Manager DBus Interface[1] to launch file manager and select a file. When this DBus interface is not present, then the current code is used, so to not break backward compatibility.

The DBus call to check if the file manager interface is present is only called once, I used a static variable for that. 

--

A quick note about how I got to this patch, I firstly cooked up a patch that used gio api to find out the handler application of the "inode/directory" mime type, so that returned me the current file manager application, and then I would just call it with the full uri of the downloaded file, but then I realize how painful linux diversity can be.. some file managers when passed a full uri would try to 'run' the file (as was thunar in xfce or konqueror in kde) while others would do the expected behaviour (open folder and select file) only when a specific flag was passed on the command line, so this was a file manager hell but..

fortunately, the DBus file manager interface[1] is the perfect solution to this, defining a common and concrete interface to open and reveal a file in the default file manager, although currently is only implemented by nautilus, it was agreed on FreeDesktop list by kde and xfce too, so when they happen to implement it they will get this nice "open and reveal" behaviour from firefox but in the meantime they will get the current code which just opens the folder.

[1] http://www.freedesktop.org/wiki/Specifications/file-manager-interface/

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 813885 [details] [diff] [review]
0001-Bug-417952-Open-Containing-Folder-doesn-t-highlight-.patch

Review of attachment 813885 [details] [diff] [review]:
-----------------------------------------------------------------

sending Feedback? to paolo, I'm not sure how relevant this code is in the new downloads API, if it's we can get feedback from karlt on the actual approach

Comment 14

[Nelson Benítez León]

Attachment: screencast of firefox with patch applied, in fedora 19

Comment 15

[:Paolo Amadini]

Comment on attachment 813885 [details] [diff] [review]
0001-Bug-417952-Open-Containing-Folder-doesn-t-highlight-.patch

Hey, thanks a lot for this patch! It looks good and this is something we want to implement. The "reveal" method is used by the new Downloads API as well.

As I'm not familiar with these system interfaces, I'm requesting feedback to karlt as Marco suggested. Karl, see comment 12 for an overview of what the patch does and some reasoning.

Comment 16

[Karl Tomlinson (:karlt)]

Sounds good, thanks very much.
I'll see if I can have a more thorough look early next week.

Comment 17

[Karl Tomlinson (:karlt)]

Comment on attachment 813885 [details] [diff] [review]
0001-Bug-417952-Open-Containing-Folder-doesn-t-highlight-.patch

Thank you for working out how to do this, Nelson.  This looks like the right
approach.

Putting this in the nsGIOService is fine, even though the current
implementation doesn't use GIO, because GIO's GDBus will be the preferred DBus
library once Mozilla is no longer supporting older systems, and the method is
similar to other existing methods on nsGIOService.

The interface can be simplified to just one additional method on nsIGIOService
that returns NS_ERROR_NOT_AVAILABLE or similar when FileManager1 is not
available.

ListActivatableNames doesn't sound like what we want here.  I assume that is
looking for /usr/share/dbus-1/services/org.freedesktop.FileManager1.service,
but the interface may be provided by a non activatable application such as a
daemon in the desktop environment.  That is in fact the appropriate way to
support the interface because it means that different applications can be
selected by the user [1].  What Nautilus does [2][3] is unfortunate because it
means that people that don't want Nautilus used to reveal a file will have to
uninstall nautilus from the system, removing the option of other users using
Nautilus.

The simplest approach would be to use the synchronous dbus_g_proxy_call() so
that we know whether the call succeeds.  Given this particular use of DBus
where the user is wanting to open a new application, or at least a new window,
which is going to take time, the additional round trip time via the DBus
daemon is not going to be major.

Perhaps the call could set a flag if it fails, in order to save checking the
DBus call each time for systems without FileManager1, or perhaps there is a
way to register for notifications when the interface becomes available, but
that may be more trouble than it is worth.  Similarly I imagine there is a
notification when the interface is no longer available (the "destroy" signal
maybe?) so the call would need only be synchronous the first time, but that
can be a future optimization when necessary.

Gecko file style is to use spaces for indentation, with no tabs.

Call dbus_connection_set_exit_on_disconnect(dbusConnection, false) in case
this is the first use of the connection.

>+  char *uris[2] = { (char*)PromiseFlatCString(uri).get(), NULL };

The PromiseFlatCString usage is not good because the pointer is used after the
PromiseFlatCString is destroyed.
I expect uri.get() can be used here.

Please use const_cast instead of (char*), or, perhaps better, make this
const char *uris[2].

(In reply to Nelson Benítez León from comment #12)
>  some file managers when
> passed a full uri would try to 'run' the file (as was thunar in xfce or
> konqueror in kde)

I was surprised by that behavior too.  I think there is a KDE bug report on
that.

[1] http://lists.freedesktop.org/archives/xdg/2011-May/011971.html
[2] https://mail.gnome.org/archives/commits-list/2011-December/msg05097.html
[3] https://git.gnome.org/browse/nautilus/tree/data/Makefile.am?id=b3434e8bec9231b21f34fbe4bfc5e05b8d2e592b#n28

Comment 18

[Nelson Benítez León]

(In reply to Karl Tomlinson (:karlt) from comment #17)
> Comment on attachment 813885 [details] [diff] [review]
> 0001-Bug-417952-Open-Containing-Folder-doesn-t-highlight-.patch
> 
> Thank you for working out how to do this, Nelson.  This looks like the right
> approach.

You're welcome!, and sorry for getting back to you after a couple weeks but
I've been busy lately with only some spare time on weekends.

> Putting this in the nsGIOService is fine, even though the current
> implementation doesn't use GIO, because GIO's GDBus will be the preferred
> DBus
> library once Mozilla is no longer supporting older systems, and the method is
> similar to other existing methods on nsGIOService.
> 
> The interface can be simplified to just one additional method on
> nsIGIOService
> that returns NS_ERROR_NOT_AVAILABLE or similar when FileManager1 is not
> available.

Done.

> ListActivatableNames doesn't sound like what we want here.  I assume that is
> looking for /usr/share/dbus-1/services/org.freedesktop.FileManager1.service,
> but the interface may be provided by a non activatable application such as a
> daemon in the desktop environment.  That is in fact the appropriate way to
> support the interface because it means that different applications can be
> selected by the user [1].  What Nautilus does [2][3] is unfortunate because
> it
> means that people that don't want Nautilus used to reveal a file will have to
> uninstall nautilus from the system, removing the option of other users using
> Nautilus.

Yes you're right, although file managers are a somewhat special case of a program
very attached to its desktop environment, but they should still play well with
other file managers. Thanks for the links.

> The simplest approach would be to use the synchronous dbus_g_proxy_call() so
> that we know whether the call succeeds.  Given this particular use of DBus
> where the user is wanting to open a new application, or at least a new
> window,
> which is going to take time, the additional round trip time via the DBus
> daemon is not going to be major.

Done, using dbus_g_proxy_call() now.

> Perhaps the call could set a flag if it fails, in order to save checking the
> DBus call each time for systems without FileManager1, or perhaps there is a

Done.

> way to register for notifications when the interface becomes available, but
> that may be more trouble than it is worth.  Similarly I imagine there is a
> notification when the interface is no longer available (the "destroy" signal
> maybe?) so the call would need only be synchronous the first time, but that
> can be a future optimization when necessary.

I left this out of the patch to keep it simple, but as you said it can be added
in a future update.

> Gecko file style is to use spaces for indentation, with no tabs.
> 
> Call dbus_connection_set_exit_on_disconnect(dbusConnection, false) in case
> this is the first use of the connection.

Done.

> >+  char *uris[2] = { (char*)PromiseFlatCString(uri).get(), NULL };
> 
> The PromiseFlatCString usage is not good because the pointer is used after
> the
> PromiseFlatCString is destroyed.
> I expect uri.get() can be used here.
> 
> Please use const_cast instead of (char*), or, perhaps better, make this
> const char *uris[2].

Done.

> (In reply to Nelson Benítez León from comment #12)
> >  some file managers when
> > passed a full uri would try to 'run' the file (as was thunar in xfce or
> > konqueror in kde)
> 
> I was surprised by that behavior too.  I think there is a KDE bug report on
> that.

Comment 19

[Nelson Benítez León]

Attachment: 0001-Bug-417952-Open-Containing-Folder-doesn-t-highlight-.patch

Updated patch. Created from FIREFOX_AURORA_27_BASE tag.

Comment 20

[Karl Tomlinson (:karlt)]

Comment on attachment 826479 [details] [diff] [review]
0001-Bug-417952-Open-Containing-Folder-doesn-t-highlight-.patch

Thanks.  This is good.  Just some minor things:

>+  DBusGConnection* mDBusConnection = nullptr;
>+  DBusConnection* dbusConnection = nullptr;
>+  DBusGProxy* mDBusProxy = nullptr;

>+  gboolean rv_dbus_call;

Gecko uses the 'm' prefix on variables if they are member variables, but these
are local variables, so please rename mDBusConnection and mDBusProxy to something without the prefix, but still starting with a lower-case letter.

The initialization is unnecessary here, but this is C++ code, so these can be
declared when they are first set, and that is Gecko style. e.g.

DBusGConnection* mDBusConnection = dbus_g_bus_get(DBUS_BUS_SESSION, &error);

Initializing out parameters, as you have with |error|, is often good
(and may be necessary even - I haven't checked).

>+  nsAutoCString uri("file://");
>+  uri.Append(aUri);

Please use g_filename_to_uri(aPath, nullptr, nullptr) to do any necessary
escaping.

>+  const char *uris[2] = { (const char*) uri.get(), NULL };

The (const char*) can be removed now, I assume.

Also, please use nullptr instead of NULL as nullptr provides more type safety
than a plain 0.

>+    } else if (giovfs && giovfs->OrgFreedesktopFileManager1ShowItems(mPath) == NS_OK) {

Gecko style is usually to write

   NS_SUCCEEDED(giovfs->OrgFreedesktopFileManager1ShowItems(mPath))

>   [noscript] void    showURIForInput(in ACString uri);
>+
>+  /* Open uri in file manager using org.freedesktop.FileManager1 interface */
>+  [noscript] void    orgFreedesktopFileManager1ShowItems(in ACString uri);

showURIForInput set a bad precedent here because the parameter is a path, not a
uri.

Please name the parameter for the new method "path", and update the comment.
I assume that is easier than changing the caller to pass a uri.

Comment 21

[Nelson Benítez León]

Attachment: 0001-Bug-417952-Open-Containing-Folder-doesn-t-highlight-.patch

(In reply to Karl Tomlinson (:karlt) from comment #20)
> Comment on attachment 826479 [details] [diff] [review]
> 0001-Bug-417952-Open-Containing-Folder-doesn-t-highlight-.patch
> 
> Thanks.  This is good.  Just some minor things:

Updated patch with those things corrected. :-)

Comment 22

[Nelson Benítez León]

Hi Karl, is this now ok to commit? if so, could you commit for me? (I don't have commit rights).. Thanks!

Comment 23

[Marco Bonardo [:mak]]

Tree sheriffs can commit the patch for you, just by setting checkin-needed keyword

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fc52820456d

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5fc52820456d

Comment 26

[Nelson Benítez León]

Thank you!

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Congrat!