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 646584 - Should not use .gnome2
Should not use .gnome2
Status: RESOLVED FIXED
Product: nautilus
Classification: Core
Component: general
3.3.x
Other Linux
: Normal normal
: ---
Assigned To: Nautilus Maintainers
Nautilus Maintainers
Depends on:
Blocks: 523057
 
 
Reported: 2011-04-03 01:35 UTC by Cosimo Cecchi
Modified: 2012-06-08 10:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Remove gtk accel map functionality (3.76 KB, patch)
2012-04-26 00:37 UTC, William Jon McCann
committed Details | Review
Don't trust .desktop files in .gnome2 (1.52 KB, patch)
2012-04-26 00:37 UTC, William Jon McCann
committed Details | Review
Migrate .gnome2 scripts dir to use XDG data dir (3.09 KB, patch)
2012-04-26 00:37 UTC, William Jon McCann
committed Details | Review
Revert "Remove gtk accel map functionality" (3.84 KB, patch)
2012-05-17 10:48 UTC, Holger Berndt
committed Details | Review
Migrate accels file to use XDG user config dir (11.66 KB, patch)
2012-05-17 10:48 UTC, Holger Berndt
needs-work Details | Review
Migrate accels file to use XDG user config dir (10.54 KB, patch)
2012-05-18 20:03 UTC, Holger Berndt
committed Details | Review

Description Cosimo Cecchi 2011-04-03 01:35:46 UTC
Currently we use it for scripts and for the accels file. I think we should move them to XDG_DATA_DIR/nautilus/scripts/ and XDG_CONFIG_DIR/nautilus respectively.
Comment 1 Sebastian 2012-02-11 22:05:00 UTC
Any work on this being done at the moment?
Comment 2 Cosimo Cecchi 2012-02-14 19:30:44 UTC
(In reply to comment #1)
> Any work on this being done at the moment?

Not at the moment; patches welcome.
Comment 3 William Jon McCann 2012-04-26 00:37:32 UTC
Created attachment 212839 [details] [review]
Remove gtk accel map functionality
Comment 4 William Jon McCann 2012-04-26 00:37:35 UTC
Created attachment 212840 [details] [review]
Don't trust .desktop files in .gnome2

gnome-panel no longer stores files there.
Comment 5 William Jon McCann 2012-04-26 00:37:37 UTC
Created attachment 212841 [details] [review]
Migrate .gnome2 scripts dir to use XDG data dir
Comment 6 Sebastian 2012-04-26 08:41:39 UTC
Review of attachment 212841 [details] [review]:

I would suggest to use past tense for the message:
tried migrating -> has migrated

Since afterwards the files have already been moved. Additionally I would add:

It is save to delete this directory now.

Besides that the patch is great.
Comment 7 William Jon McCann 2012-04-26 11:52:49 UTC
I'm using the same messages used elsewhere in nautilus. And the second comment is wrong because it is only added in the case where the directory was *not* moved for some reason.
Comment 8 Cosimo Cecchi 2012-04-26 12:46:41 UTC
Review of attachment 212839 [details] [review]:

Yes, looks good.
Comment 9 Cosimo Cecchi 2012-04-26 12:48:03 UTC
Review of attachment 212840 [details] [review]:

Looks good.
Comment 10 Cosimo Cecchi 2012-04-26 13:05:30 UTC
Review of attachment 212841 [details] [review]:

Looks good to me.
Comment 11 Christian Persch 2012-04-26 15:58:13 UTC
+			if (g_mkdir_with_parents (parent_dir, 0755) == 0) {

This is wrong; if the toplevel part (g_get_user_data_dir()) doesn't exist yet, it will be created 0755 but the xdg spec says it must be 0700.
Comment 12 Cosimo Cecchi 2012-04-26 16:19:58 UTC
(In reply to comment #11)
> +            if (g_mkdir_with_parents (parent_dir, 0755) == 0) {
> 
> This is wrong; if the toplevel part (g_get_user_data_dir()) doesn't exist yet,
> it will be created 0755 but the xdg spec says it must be 0700.

Hmm, I guess the expectation of the code is that this would only create the nautilus directory inside g_get_user_data_dir(), but if g_get_user_data_dir() itself doesn't exist (which I doubt would happen in real circumstances) you're right, and it might be created with the wrong permissions.
Also, the old code below calls g_mkdir_with_parents() too with 0755, which would fail for the same reason. Since having the scripts directory with a more restrictive permission mask doesn't really hurt, I changed both to use 0700 now.
Comment 13 Holger Berndt 2012-05-06 19:59:11 UTC
What was the reason for removing the accel map functionality alltogether? This was certainly not necessary to fix this bug.
Comment 14 Cosimo Cecchi 2012-05-08 17:29:56 UTC
(In reply to comment #13)
> What was the reason for removing the accel map functionality alltogether? This
> was certainly not necessary to fix this bug.

It's obscure, confusing, totally undiscoverable and there's really no reason to have it in Nautilus.
Comment 15 Holger Berndt 2012-05-08 19:42:16 UTC
(In reply to comment #14)
> It's obscure, confusing, totally undiscoverable and there's really no reason to
> have it in Nautilus.

It's actually a very nice, unobtrusive desktop feature. Nautilus is not the only application that supports it (well, supported it). The desktop as a whole is less consistent now.

It's not harming you at all, but I use it a lot in Nautilus to assign keyboard shortcuts to my Nautilus Skripts. Why are you breaking my work flow for no reason or benefit whatsoever?

I just realize that your characterization would be just as fitting (or not-fitting) for Nautilus Skripts as a whole. I'm glad they were not ripped out as a "solution" to this report as well.

I must admit I find it rather annoying to find this kind of breakage by coincidence. I was the one who contributed that feature. I took the effort and time to make a patch, I went trough two review iterations until Alex was happy, and now, I find it removed without comment, on a totallly unrelated bug report. Surprising. And weird.
Comment 16 Cosimo Cecchi 2012-05-08 20:28:22 UTC
(In reply to comment #15)

> It's actually a very nice, unobtrusive desktop feature. Nautilus is not the
> only application that supports it (well, supported it). The desktop as a whole
> is less consistent now.

The functionality brings in inconsistency between applications by its very nature (since it allows to override designated keybindings which are usually assigned with consistency with other applications in mind), and is gradually being removed by other applications in GNOME.

> It's not harming you at all, but I use it a lot in Nautilus to assign keyboard
> shortcuts to my Nautilus Skripts. Why are you breaking my work flow for no
> reason or benefit whatsoever?
> 
> I just realize that your characterization would be just as fitting (or
> not-fitting) for Nautilus Skripts as a whole. I'm glad they were not ripped out
> as a "solution" to this report as well.

I have no plans to remove the scripts functionality, I don't know what would make you think I do - but I'm sorry to break your workflow, it was not my intention.
I'd argue that if the goal is being able to assign custom keybindings to scripts, custom accels are a terrible way to do it, and scripts should be able to specify that in a different and more discoverable way. Bug 349989 comes to mind here, and we could probably come up with a solution for scripts too.

> I must admit I find it rather annoying to find this kind of breakage by
> coincidence. I was the one who contributed that feature. I took the effort and
> time to make a patch, I went trough two review iterations until Alex was happy,
> and now, I find it removed without comment, on a totallly unrelated bug report.
> Surprising. And weird.

Code comes and goes, that's how things have always worked...I don't see this being useful as a general option, it doesn't work with application menus, it breaks everything if it's enabled by mistake, and it's a very suboptimal fit for customizing scripts keybindings.
I'm very open to suggestions to improve the latter, though.
Comment 17 Holger Berndt 2012-05-08 21:03:27 UTC
(In reply to comment #16)
> and is gradually
> being removed by other applications in GNOME.

Okay, of course, if you remove the feature from all applications, you're being consistent again (but regressed).

> I'd argue that if the goal is being able to assign custom keybindings to
> scripts, custom accels are a terrible way to do it, and scripts should be able
> to specify that in a different and more discoverable way. Bug 349989 comes to
> mind here, and we could probably come up with a solution for scripts too.

The feature you just removed did allow what the reporter in bug 349989 wants, circumventing the confict issue. There've been several discussions about that, see for example
https://mail.gnome.org/archives/nautilus-list/2009-May/msg00007.html

> Code comes and goes, that's how things have always worked...

A recent discussion on d-d-l comes to my mind:
https://mail.gnome.org/archives/desktop-devel-list/2012-January/msg00136.html

Of course I don't want to imply that maintainers are accountable towards the occasional (unfortunately far-to-seldom, in my case) contributor. I'm just mentioning that this is a severe regression for me, and I'm not seeing the "succiciently large step forward" that it enables. Oppinions may differ, of course.

> I don't see this
> being useful as a general option, it doesn't work with application menus, it
> breaks everything if it's enabled by mistake, and it's a very suboptimal fit
> for customizing scripts keybindings.

If you're interested in usage scenarios: Besides for Skripts, I also use this feature to bind "Move to trash" to the Del-key again, after I lost work by accidentially deleting stuff circumventing the Trash. I can tell apart Del and Shift-Del, but I keep mixing up Ctrl-Del and Shift-Del. They are just too close together.

But that's a side-issue. For me personally, the Skripts are really the main point.

> I'm very open to suggestions to improve the latter, though.

Your responsiveness and openness is really appreciated.
Comment 18 Cosimo Cecchi 2012-05-08 21:26:42 UTC
(In reply to comment #17)

> The feature you just removed did allow what the reporter in bug 349989 wants,
> circumventing the confict issue. There've been several discussions about that,
> see for example
> https://mail.gnome.org/archives/nautilus-list/2009-May/msg00007.html

It could have been incidentally used for that too, yes.
If asked, though, I think I would have said "we do not support it" rather than advising going trough the whole can-change-accels craziness...

> > Code comes and goes, that's how things have always worked...
> 
> A recent discussion on d-d-l comes to my mind:
> https://mail.gnome.org/archives/desktop-devel-list/2012-January/msg00136.html
> 
> Of course I don't want to imply that maintainers are accountable towards the
> occasional (unfortunately far-to-seldom, in my case) contributor. I'm just
> mentioning that this is a severe regression for me, and I'm not seeing the
> "succiciently large step forward" that it enables. Oppinions may differ, of
> course.

I don't know...the price of supporting obscure features like this, even if they're small, is usually quite high compared to other parts of the code, since it's code not being tested that might break when the rest moves.

> If you're interested in usage scenarios: Besides for Skripts, I also use this
> feature to bind "Move to trash" to the Del-key again, after I lost work by
> accidentially deleting stuff circumventing the Trash. I can tell apart Del and
> Shift-Del, but I keep mixing up Ctrl-Del and Shift-Del. They are just too close
> together.
> 
> But that's a side-issue. For me personally, the Skripts are really the main
> point.

Yeah, let's not mix this with the Ctrl+Del vs. Del flamewar.

> > I'm very open to suggestions to improve the latter, though.
> 
> Your responsiveness and openness is really appreciated.

If you're willing to write a patch to revive this, make it use the correct XDG location, handling the migration to the new location and everything, I will keep it in until we have a better way to assign keybindings to scripts, (or until the accel map feature itself is eventually deprecated from GTK).
I won't write that patch or fix if it breaks though :)
Comment 19 Holger Berndt 2012-05-09 22:29:03 UTC
Great, I'll try to get to that soon.
Comment 20 Holger Berndt 2012-05-17 10:48:11 UTC
Created attachment 214241 [details] [review]
Revert "Remove gtk accel map functionality"

This reverts commit b3fc292bceb64c1a4799df320bd235b314466aef.
Comment 21 Holger Berndt 2012-05-17 10:48:16 UTC
Created attachment 214242 [details] [review]
Migrate accels file to use XDG user config dir

Also, save the accel map on shutdown, as Nautilus is now typically
not run as a service anymore, but shutdown cleanly.
Comment 22 Cosimo Cecchi 2012-05-18 15:24:33 UTC
Review of attachment 214242 [details] [review]:

Looks mostly good; I put some comments below.

::: libnautilus-private/nautilus-file-utilities.c
@@ +127,3 @@
 nautilus_get_accel_map_file (void)
 {
+	return g_build_filename (g_get_user_config_dir(), "nautilus", "accels", NULL);

Missing space before bracket.

::: libnautilus-private/nautilus-ui-utilities.c
@@ +134,3 @@
+char *
+nautilus_escape_action_name (const char *action_name,
+		    const char *prefix)

Wrong indentation

::: src/nautilus-application.c
@@ +1154,3 @@
+
+		/* base part of accel */
+

Coding style: missing spaces

@@ +1160,3 @@
+		/* new script directory, escaped */
+		tmp = nautilus_get_scripts_directory_path ();
+		 * add a migrated one. */

Ditto

@@ +1166,3 @@
+
+		/* script path relative to scripts directory */
+		GString *new_accel_path;

Ditto

@@ +1221,3 @@
+				 * on startup instead of reading the old one and possibly
+				 * being stuck with it - so the failure case is not handled. */
+								       ".gnome2",

It's simpler to just use g_rename() directly on the file paths, instead of going trough g_file_move() here.

@@ +1226,3 @@
+				g_object_unref (old_accel_map_file);
+
+		}

Shouldn't performed_migration be set to FALSE here if g_file_move/g_rename failed?

@@ +1238,3 @@
+	g_free (accel_map_filename);
+
+			if (g_mkdir_with_parents (parent_dir, 0700) == 0) {

Missing space after if.

@@ +1247,3 @@
+		/* save map immediately */
+		save_of_accel_map_requested = TRUE;
+				accel_map_file = g_file_new_for_path (accel_map_filename);

Missing space before bracket.

::: src/nautilus-main.c
@@ +115,3 @@
+ 		gtk_accel_map_save (accel_map_filename);
+ 		g_free (accel_map_filename);
+ 	}

You should do this in the quit_mainloop() NautilusApplication handler instead. Actually, we should already call nautilus_application_save_accel_map() there, why isn't it enough?
Comment 23 Cosimo Cecchi 2012-05-18 15:25:25 UTC
Review of attachment 214241 [details] [review]:

Looks fine to commit, but it should go in together with the other patch when it's ready.
Comment 24 Holger Berndt 2012-05-18 20:03:15 UTC
Created attachment 214398 [details] [review]
Migrate accels file to use XDG user config dir
Comment 25 Holger Berndt 2012-05-18 20:03:30 UTC
Review of attachment 214242 [details] [review]:

Thanks a lot for the fast review. I'm attaching an updated patch, except
for the following points which are either unclear or seem wrong to me:

::: libnautilus-private/nautilus-ui-utilities.c
@@ +134,3 @@
+char *
+nautilus_escape_action_name (const char *action_name,
+		    const char *prefix)

Function argument indentation seems to be used inconsistently accross Nautilus, and isn't mentioned in HACKING or style-guide.html. What is the correct style?

::: src/nautilus-application.c
@@ +1221,3 @@
+				 * on startup instead of reading the old one and possibly
+				 * being stuck with it - so the failure case is not handled. */
+				g_file_move (old_accel_map_file, accel_map_file, 0, NULL, NULL, NULL, NULL);

No, that would change semantics. g_rename() doesn't work accross filesystem boundaries, g_file_move() falls back to copy/unlink in that case.

In fact, it would fail on my machine - I have most of /home on a harddisc, but tend to put config stuff onto an SSD, so ~/.config is not on the same filesystem as most other parts in $HOME.

I was thinking about also fixing this for the migration of the scripts directory, but didn't do so as scripts could potentially be big, making g_file_move() possibly slow. However, thinking about it again, g_file_move() works just as fast within
filesystem boundaries, so in my oppinion is should be used there too.

@@ +1226,3 @@
+				g_object_unref (old_accel_map_file);
+
+				performed_migration = TRUE;

Leaving it at TRUE is not doing any harm, but you're right that there won't be any accel paths to update anyways if moving failed. I'll change it.

::: src/nautilus-main.c
@@ +115,3 @@
+ 		gtk_accel_map_save (accel_map_filename);
+ 		g_free (accel_map_filename);
+ 	}

I think it is - I missed out on this. Part in main() removed from the patch.
Comment 26 Cosimo Cecchi 2012-05-21 20:39:35 UTC
(In reply to comment #25)

> Function argument indentation seems to be used inconsistently accross Nautilus,
> and isn't mentioned in HACKING or style-guide.html. What is the correct style?

I know Nautilus is not always consistent, but generally the style we settled on is to have one argument per line, aligned to the function name, and always using 8-space tabs.

> ::: src/nautilus-application.c
> @@ +1221,3 @@
> +                 * on startup instead of reading the old one and possibly
> +                 * being stuck with it - so the failure case is not handled.
> */
> +                g_file_move (old_accel_map_file, accel_map_file, 0, NULL,
> NULL, NULL, NULL);
> 
> No, that would change semantics. g_rename() doesn't work accross filesystem
> boundaries, g_file_move() falls back to copy/unlink in that case.
> 
> In fact, it would fail on my machine - I have most of /home on a harddisc, but
> tend to put config stuff onto an SSD, so ~/.config is not on the same
> filesystem as most other parts in $HOME.

Okay, makes sense then (even though as you note, other code in Nautilus and GNOME in general assume the home directory is not divided among different file systems).
Comment 27 Cosimo Cecchi 2012-05-21 20:40:04 UTC
Review of attachment 214398 [details] [review]:

Looks good.
Comment 28 Holger Berndt 2012-05-23 19:08:32 UTC
Commited a version with fixed indent.

Thanks again for the review!

Attachment 214241 [details] pushed as c857f6b - Revert "Remove gtk accel map functionality"
Attachment 214398 [details] pushed as f60ee69 - Migrate accels file to use XDG user config dir