GNOME Bugzilla – Bug 646584
Should not use .gnome2
Last modified: 2012-06-08 10:27:00 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.
Any work on this being done at the moment?
(In reply to comment #1) > Any work on this being done at the moment? Not at the moment; patches welcome.
Created attachment 212839 [details] [review] Remove gtk accel map functionality
Created attachment 212840 [details] [review] Don't trust .desktop files in .gnome2 gnome-panel no longer stores files there.
Created attachment 212841 [details] [review] Migrate .gnome2 scripts dir to use XDG data dir
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.
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.
Review of attachment 212839 [details] [review]: Yes, looks good.
Review of attachment 212840 [details] [review]: Looks good.
Review of attachment 212841 [details] [review]: Looks good to me.
+ 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.
(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.
What was the reason for removing the accel map functionality alltogether? This was certainly not necessary to fix this bug.
(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.
(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.
(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.
(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.
(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 :)
Great, I'll try to get to that soon.
Created attachment 214241 [details] [review] Revert "Remove gtk accel map functionality" This reverts commit b3fc292bceb64c1a4799df320bd235b314466aef.
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.
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?
Review of attachment 214241 [details] [review]: Looks fine to commit, but it should go in together with the other patch when it's ready.
Created attachment 214398 [details] [review] Migrate accels file to use XDG user config dir
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.
(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).
Review of attachment 214398 [details] [review]: Looks good.
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