! Please note that this is a snapshot of our old Bugzilla server, which is read only since May 29, 2020. Please go to gitlab.xfce.org for our new server !
xdg user directory compatibility patches
Status:
RESOLVED: FIXED

Comments

Description Andrea Santilli 2008-09-08 09:04:41 CEST
Created attachment 1805 
patch for libxfce4util

These patches have a long story... I first submitted them to redhat bugzilla (https://bugzilla.redhat.com/show_bug.cgi?id=457740) cos I found annoying that thunar could not detect the xdg user directories.

The guy there liked them so I thought it was a good idea to post them upstream.
Then I had to modify them for xfce 4.5.0 and I also fixed some little things.

The patches can make thunar (and other applications) get the xdg user directories and were made against 4.5.0-rev27605

Changes on libxfce4util
This was made to make libxfce4util provide the function xfce_get_user_special_dir(). If you're using glib >= 2.14 then that call is just a macro that redirects to g_get_user_special_dir(). Otherwise it completely reimplements such function by reading the file ~/.config/user-dirs.dirs.
This patch became necessary since xfce 4.6.x will be based on glib 2.12 and g_get_user_special_dir is in glib since version 2.14

Changes on thunar:
- It looks for the localized Desktop and Templates directory names.
- It adds the user directories to the Go menu (of course the menu item names are still untranslated). About this point I'm trying to make Thunar hide those menu items when xfce_get_user_special_dir() returns NULL or $HOME, but I'm still having some problems with that.

Changes on xfdesktop:
- Gets the templates from the localized template directory and shows the localized desktop directory contents.

Other applications might want to use those directories too.

In the meanwhile I'm also writing an application for the settings manager that can set those directories through a gui and save the changes in the user-dirs.dirs file
Comment 1 Andrea Santilli 2008-09-08 09:05:24 CEST
Created attachment 1806 
patch for thunar
Comment 2 Andrea Santilli 2008-09-08 09:05:49 CEST
Created attachment 1807 
patch for xfdesktop
Comment 3 Andrea Santilli 2008-09-08 09:06:02 CEST
These patches have a long story... I first submitted them to redhat bugzilla (https://bugzilla.redhat.com/show_bug.cgi?id=457740) cos I found annoying that thunar could not detect the xdg user directories.

The guy there liked them so I thought it was a good idea to post them upstream.
Then I had to modify them for xfce 4.5.0 and I also fixed some little things.

The patches can make thunar (and other applications) get the xdg user directories and were made against 4.5.0-rev27605

Changes on libxfce4util
This was made to make libxfce4util provide the function xfce_get_user_special_dir(). If you're using glib >= 2.14 then that call is just a macro that redirects to g_get_user_special_dir(). Otherwise it completely reimplements such function by reading the file ~/.config/user-dirs.dirs.
This patch became necessary since xfce 4.6.x will be based on glib 2.12 and g_get_user_special_dir is in glib since version 2.14

Changes on thunar:
- It looks for the localized Desktop and Templates directory names.
- It adds the user directories to the Go menu (of course the menu item names are still untranslated). About this point I'm trying to make Thunar hide those menu items when xfce_get_user_special_dir() returns NULL or $HOME, but I'm still having some problems with that.

Changes on xfdesktop:
- Gets the templates from the localized template directory and shows the localized desktop directory contents.

Other applications might want to use those directories too.

In the meanwhile I'm also writing an application for the settings manager that can set those directories through a gui and save the changes in the user-dirs.dirs file
Comment 4 Andrea Santilli 2008-09-08 09:06:41 CEST
oops sorry I sent the same text twice.
Comment 5 Tomasz Paweł Gajc 2008-09-08 13:57:36 CEST
*** Bug 4062 has been marked as a duplicate of this bug. ***
Comment 6 Andrea Santilli 2008-09-13 09:04:19 CEST
Created attachment 1815 
updated patch for libxfce4util
Comment 7 Andrea Santilli 2008-09-13 09:04:52 CEST
Created attachment 1816 
updated patch for thunar
Comment 8 Andrea Santilli 2008-09-13 09:06:52 CEST
hello again,
the patch is finally complete, now thunar hides the menu entries for the directories that were not defined.
I also updated the patch for libxfce4util since earlier I included in the patch 2 files that were not to be patched.
Comment 9 Brian J. Tarricone (not reading bugmail) 2008-09-13 09:47:36 CEST
As I've already said, I don't want new compat wrappers in libxfce4util for this sort of thing.  Thunar and xfdesktop should just use g_get_user_special_dir() directly, surrounded by "#if GLIB_CHECK_VERSION(2, 14, 0)".  Users with older versions of glib can do without the feature.
Comment 10 Andrea Santilli 2008-09-15 10:27:27 CEST
Sorry for my late reply.

hmm you know Brian I left that function because I believe that it could be done even better.
For example, on unix systems, I think that function should first retrieve the XDG_*_DIR environment variables and then, if those aren't set up, read ~/.config/user-dirs.dirs.

On win32, if we are really interested in supporting that plaform, those values are set in the registry, together with the directory icons too, especially on vista.

Do you think that some changes about these points would justify the inclusion of such a function in libxfce4util?

tc
Comment 11 Christian Dywan 2008-09-15 13:51:23 CEST
(In reply to comment #10)
> Sorry for my late reply.
> 
> hmm you know Brian I left that function because I believe that it could be done
> even better.
> For example, on unix systems, I think that function should first retrieve the
> XDG_*_DIR environment variables and then, if those aren't set up, read
> ~/.config/user-dirs.dirs.
> 
> On win32, if we are really interested in supporting that plaform, those values
> are set in the registry, together with the directory icons too, especially on
> vista.
> 
> Do you think that some changes about these points would justify the inclusion
> of such a function in libxfce4util?

That is no justification at all. If you think that Glib's implementation is insufficient you should really bring that up upstream instead of inventing new API here. So *please* file a bug on bugzilla.gnome.org instead and just use the upstream API conditionally here.
Comment 12 Brian J. Tarricone (not reading bugmail) 2008-09-15 18:22:50 CEST
(In reply to comment #10)
> Sorry for my late reply.
> 
> hmm you know Brian I left that function because I believe that it could be done
> even better.
> For example, on unix systems, I think that function should first retrieve the
> XDG_*_DIR environment variables and then, if those aren't set up, read
> ~/.config/user-dirs.dirs.

Are there cases where the XDG_*_DIR vars might get out of sync with ~/.config/user-dirs.dirs, and, if so, are you sure we'd actually prefer to use the env var over the values in the file?  Seems like if someone changes a var via a GUI, you'd have a "more recent" value in the file, and you'd want to use that, and not rely on the env vars.

> On win32, if we are really interested in supporting that plaform, those values
> are set in the registry, together with the directory icons too, especially on
> vista.

And glib can handle that, if it so desires.  For Xfce we don't really care all that much.  I doubt we even compile on cygwin/mingw at present (we used to, but likely things have changed since then that breaks that).

> Do you think that some changes about these points would justify the inclusion
> of such a function in libxfce4util?

Sorry, not really, no.  The glib version still seems entirely sufficient to me, and in places where it isn't, we should just file bugs against glib.
Comment 13 Andrea Santilli 2008-09-16 04:27:31 CEST
Created attachment 1822 
patch for thunar - update 2
Comment 14 Andrea Santilli 2008-09-16 04:28:12 CEST
Created attachment 1823 
patch for xfdesktop - update 2
Comment 15 Andrea Santilli 2008-09-16 04:30:43 CEST
ok the patch for libxfce4util is now dropped.
I added few autotools lines so that the users will be able to disable this features if they dont like it.
Comment 16 Brian J. Tarricone (not reading bugmail) 2008-09-16 05:07:43 CEST
No need to add any configure magic at all.  You can just use "#if GLIB_CHECK_VERSION(2, 14, 0)" in the code.

The patch is simple enough, so I just modified it myself and checked it in, with some modifications to make it a bit simpler.
Comment 17 Brian J. Tarricone (not reading bugmail) 2008-09-16 07:23:23 CEST
As for the Thunar patch, it's a bit too intrusive for my taste, has too many unnecessary #ifdefs and duplicates a lot of code.

For starters: ditch the configure option.  There's no reason why someone *wouldn't* want this set up properly, and there's an established way to disable xdg-user-dirs if they don't want it.

All the thunar_window_action_open_*() functions are basically copy-and-pastes of each other with just the directory type being different.  All of the common code should be in a function of its own that takes the directory type as a parameter.  For glib < 2.14, it should return hard-coded defaults and not bother with xdg-user-dirs at all.

(As an aside, I'd suggest something like:

#if GLIB_CHECK_VERSION(2, 14, 0)
typedef GUserDirectory ThunarUserDirectory;
#define THUNAR_USER_DIRECTORY_DESKTOP    G_USER_DIRECTORY_DESKTOP
#define THUNAR_USER_DIRECTORY_DOCUMENTS  G_USER_DIRECTORY_DOCUMENTS
/* ... and the rest... */
#else
typedef enum
{
    /* THUNAR_* enum vals, in same order as G_USER_* in gutils.h */
} ThunarUserDirectory;
#endif

... so you don't need #ifdefs all over the place.)

And then you have two options for the GtkActionEntry struct:

1.  Keep separate functions for each item, but just have those functions call your master function that does all the work depending on the dir type.
OR
2.  Use a single function for each item, and strcmp() the GtkAction name to figure out which dir type to use.

#2 has slightly smaller code size, but the extra strcmp() overhead makes me think #1 is better.

I'd also want to see the shortcuts for these dirs in the shortcuts pane made optional, probably just by hidden options in ~/.config/Thunar/thunarrc for now.  Not sure which ones should be on by default, but that's not a big deal to change later.  For glib < 2.14, all special dirs shown in the shortcut pane should be default disabled except for whatever we currently show in Thunar now.

For areas where you just have to call g_get_user_special_dir(), I'd prefer structure like this (this is what I committed for xfdesktop, decreasing your patch by half the changes):

gchar *path = NULL;

#if GLIB_CHECK_VERSION(2, 14, 0)
path = g_strdup(g_get_user_special_dir(WHATEVER));
#endif
if(!path)
    path = find_dir_in_fallback_way();

use_dir_somehow(path);
g_free(path);

Yes, you're often g_strdup()ing a string that you might just be g_free()ing soon after, but assuming that find_dir_in_fallback_way() returns allocated memory, you're avoiding an ugly bool to track whether or not it needs to be freed and are just making the code clearer and easier to understand.  And avoiding a copy-and-paste of the fallback code is good too.

This is a good start, though.  Finding the parts that need to be changed is often hardest; getting it right is just a matter of details.
Comment 18 Andrea Santilli 2008-09-20 06:30:45 CEST
Created attachment 1825 
patch for thunar - update 3

I made many things a lot simpler, the autoconf thingy is gone in favor of GLIB_CHECK_VERSION and I added a little trick to make thunar add some user directories to ~/.gtk-bookmarks in case this file wasn't previously created.

For the directory choice I took example from nautilus, thus taking:
G_USER_DIRECTORY_DOCUMENTS, G_USER_DIRECTORY_DOWNLOAD, G_USER_DIRECTORY_MUSIC, G_USER_DIRECTORY_PICTURES and G_USER_DIRECTORY_VIDEOS. These can be easily changed.

Then it saves the changes in the bookmarks file.

Everything is done while building the side pane tree model, so that the new links will show in the side pane immediately.

That seemed the most reasonable solution for me as, in case some people want to change the directories (or not have them at all), they just have to change the side pane links as usual.

And thank you very much Brian for committing the changes to xfdesktop :)
Comment 19 Stephan Arts editbugs 2008-09-29 14:33:04 CEST
add myself to CC
Comment 20 Andrea Santilli 2008-10-09 10:47:38 CEST
Created attachment 1875 
patch for thunar - update 4

This time the patch should be way faster.

Oh I also changed the way the menu entries appear:
 - with glib < 2.12 they dont appear at all
 - with glib >= 2.12 they *always* appear but if a user directory wasnt set, the entry becomes insensitive
Comment 21 Brian J. Tarricone (not reading bugmail) 2008-10-09 18:15:58 CEST
No, if the directory isn't set, it's just wasting space.  Definitely do not add in that case.
Comment 22 Christian Dywan 2008-10-09 18:52:24 CEST
I'm using the latest Thunar patch now.

I second, better don't add the items if folders are unset.

Is it intentional that the sidebar shows "Foldername" while the Go menu shows "_Desktop"? I guess that's because of the mnemonic?
To me it seems a little confusing, since I don't see Desktop when navigating through folders via Thunar or File choosers. Not sure.
Comment 23 Christian Dywan 2008-10-09 18:56:35 CEST
And note that there's the Places panel plugin, it needs support for a custom Desktop folder as well. Just that it's not forgotten.
Comment 24 Andrea Santilli 2008-10-09 19:07:42 CEST
Created attachment 1876 
patch for thunar - update 5

ok back to the previous behavior.
I also fixed a couple of slips (duh)
Comment 25 Andrea Santilli 2008-10-09 19:24:17 CEST
Christian, I know what you mean.
Right now, even after the menu entry label translation, you'd still have a (translated) "Templates" entry in the menu and something like "MyTemplatesDir" in the side pane.
The problem is: if you rename, for instance, your templates directory using a name hasn't much to do with the word "Templates" nor with its translation, and you want to look for it in the menu, how can you find it?
Hmm I don't really know what's better...
Comment 26 Andrea Santilli 2008-10-10 05:54:24 CEST
I have an idea for the menu entries but I don't know if that will make things impossible for the translators.

Let's have the menu entry labels like this:
TranslatedFolderFunctionName (ActualFolderName)

or viceversa.

About the internals, I don't like so much thunar_file_is_desktop().
Previously it was just a macro, now it needs to allocate and deallocate heap space just to do a comparison between the result of g_get_user_special_dir() and the current ThunarFile.
If you are viewing a directory with lots of subdirectories, thunar_file_is_desktop() will compare all their ThunarFile objects with the desktop path, which is a string.

I was thinking about keeping a ThunarFile for the desktop path somewhere in the app and then do a comparison with the other ThunarFile's as follows:
- compare their names
- do the same with their parents and so on

Also, for the name comparison we could avoid to call an external function (like exo_str_is_equal()) to gain a bit of speed.

What do you think?
Comment 27 Andrea Santilli 2008-10-10 06:19:08 CEST
Oh I forgot to say that if TranslatedFolderFunctionName and ActualFolderName were the same string, there we would make it show it just once :)
Comment 28 Brian J. Tarricone (not reading bugmail) 2008-10-10 06:46:25 CEST
(In reply to comment #26)
> I have an idea for the menu entries but I don't know if that will make things
> impossible for the translators.
> 
> Let's have the menu entry labels like this:
> TranslatedFolderFunctionName (ActualFolderName)
> 
> or viceversa.

Right, I had hoped this is how you'd do it.  I don't want to see the actual folder name in the shortcuts pane; I want to see the translated version of what it actually *is* (Desktop, Documents, Music, etc.).  This is definitely needed.  Perhaps we can leverage the translations in the xdg-user-dirs .po files?  Either load their message catalog directly, or merge the translations in thunar?

> About the internals, I don't like so much thunar_file_is_desktop().
> Previously it was just a macro, now it needs to allocate and deallocate heap
> space just to do a comparison between the result of g_get_user_special_dir()
> and the current ThunarFile.
> If you are viewing a directory with lots of subdirectories,
> thunar_file_is_desktop() will compare all their ThunarFile objects with the
> desktop path, which is a string.
> 
> I was thinking about keeping a ThunarFile for the desktop path somewhere in the
> app and then do a comparison with the other ThunarFile's as follows:
> - compare their names
> - do the same with their parents and so on
> 
> Also, for the name comparison we could avoid to call an external function (like
> exo_str_is_equal()) to gain a bit of speed.
> 
> What do you think?

I think you should profile it first before making wild guesses as to possible performance problems ^_~.  Since ThunarFile isn't public API, I suppose you could leave thunar_file_is_desktop() as a macro for glib < 2.14, and maybe do some inlining for glib >= 2.14.  Dunno if it really matters, tho.  Should try it.  I imagine just stat()ing a file on disk would take orders of magnitude longer than a couple memory allocations and a string comparison.

Definitely do some profiling -- with both cold and warm cache -- before trying to optimise here.  Much higher prio is to get the shortcuts behaving properly by this Saturday.  The fewer things we have to break feature freeze for after beta1, the better.
Comment 29 Andrea Santilli 2008-10-10 09:09:20 CEST
In the meanwhile I posted a very simple patch for the places plugin here:

http://bugzilla.xfce.org/show_bug.cgi?id=4461
Comment 30 Christian Dywan 2008-10-10 12:36:28 CEST
(In reply to comment #28)
> (In reply to comment #26)
> > I have an idea for the menu entries but I don't know if that will make things
> > impossible for the translators.
> > 
> > Let's have the menu entry labels like this:
> > TranslatedFolderFunctionName (ActualFolderName)
> > 
> > or viceversa.
> 
> Right, I had hoped this is how you'd do it.  I don't want to see the actual
> folder name in the shortcuts pane; I want to see the translated version of
> what it actually *is* (Desktop, Documents, Music, etc.).

That doesn't solve the problem of finding your actual folder on the disk unless you use the localized name inside the file view, like OSX does.

I would think either
1. Consequently always use "FolderName", which would normally be a localized version of each folder's purpose anyway
2. Or use the localized name everywhere in Thunar.

>  Perhaps we can leverage the translations in the xdg-user-dirs .po files? 
> Either load their message catalog directly, or merge the translations in
> thunar?

Using the xdg message catalog seems like the best idea.

Oh, and please don't forget about the following rule:

"To disable a directory, point it to the homedir."

(As stated in http://freedesktop.org/wiki/Software/xdg-user-dirs)
Comment 31 Brian J. Tarricone (not reading bugmail) 2008-10-10 19:58:02 CEST
(In reply to comment #30)
> (In reply to comment #28)
> > Right, I had hoped this is how you'd do it.  I don't want to see the actual
> > folder name in the shortcuts pane; I want to see the translated version of
> > what it actually *is* (Desktop, Documents, Music, etc.).
> 
> That doesn't solve the problem of finding your actual folder on the disk unless
> you use the localized name inside the file view, like OSX does.

I'm not addressing that problem.  The rest of Andrea's patch does that with g_get_user_special_dir().  I'm solely talking about how it should look in the UI.  As an example, I want the string displayed in the UI for the desktop folder to always read "Desktop" (or the translated equivalent) regardless of what the actual folder name on disk is (it could be XDG_DESKTOP_DIR=$HOME/foobarbaz, but the UI should still say "Desktop").

> >  Perhaps we can leverage the translations in the xdg-user-dirs .po files? 
> > Either load their message catalog directly, or merge the translations in
> > thunar?
> 
> Using the xdg message catalog seems like the best idea.

Yeah, agreed.  I'm just not sure how to do that best.  And we don't require that the xdg-user-dirs package is installed, so, if it isn't, users won't get localised dirs.

> Oh, and please don't forget about the following rule:
> 
> "To disable a directory, point it to the homedir."
> 
> (As stated in http://freedesktop.org/wiki/Software/xdg-user-dirs)

Ooh, I'd missed that.  I'm not sure what behavior we should take in that case, tho.  Hide the shortcut?  But maybe they want Documents to be $HOME, shouldn't we still display Documents?  Then again, they already have a $HOME shortcut, so maybe not.  Yeah, I guess not.  Andrea, can your patch handle that?  If g_get_user_special_dir() ever returns $HOME, we shouldn't show a shortcut for that one.
Comment 32 Andrea Santilli 2008-10-11 20:34:46 CEST
Created attachment 1882 
patch for thunar - update 6

Ok the directory function names are now translated.
The side panel now shows the function name for the user directories, even for the desktop. You need to delete ~/.gtk-bookmarks to let thunar rebuild it.
Same thing for the menu, except that if an actual folder name differs from the name of its role, it shows the label as "FunctionName [ActualName]".
Also the directories pointing to $HOME are now disabled, with the exception of the desktop and the templates because we need them anyway.
Comment 33 Andrea Santilli 2008-10-11 20:37:41 CEST
Oh I forgot to say that the locale for the directory names is fetched from ~/.config/user-dirs.locale :)
Comment 34 Andrea Santilli 2008-10-11 20:43:40 CEST
Sorry, I forgot to tell about another detail.
When the desktop and the templates directories point to $HOME, thunar makes them work anyway. It just won't show them in the sidebar nor in the menu.
Comment 35 Andrea Santilli 2008-10-11 21:46:38 CEST
Created attachment 1883 
patch for thunar - update 7

Sorry again, I found and fixed a problem that could make thunar segfault.
Now it's fine for real.
Comment 36 Andrea Santilli 2008-10-12 11:02:24 CEST
Created attachment 1885 
patch for thunar - update 8

No code change, I just fixed the previous patch to use the existing code style.
I also added an entry to ChangeLog as requested in the HACKING file.
Comment 37 Christian Dywan 2008-10-12 13:19:22 CEST
(In reply to comment #36)
> Created an attachment (id=1885) [details]
> patch for thunar - update 8
> 
> No code change, I just fixed the previous patch to use the existing code style.
> I also added an entry to ChangeLog as requested in the HACKING file.

Just tried the latest patch, segfaultet instantly:

#0  0x00007f545c2a137d in g_build_path_va () from /usr/lib/libglib-2.0.so.0
#1  0x00007f545c2a1659 in g_build_filename () from /usr/lib/libglib-2.0.so.0
#2  0x0000000000457198 in special_dir_menu_entry_setup (window=0x12a30c0)
    at thunar-window.c:653
#3  0x00000000004593be in thunar_window_init (window=0x12a30c0)
    at thunar-window.c:764

I had no Templates folder defined. With one it doesn't segfault.

My Desktop being $HOME, the Go menu doesn't show it anymore, but I'm still seeing it in the shortcuts panel.

The "Special folder name\t[Real folder name]" style looks odd, particularly the tab in there and the rectangular brackets.
Comment 38 Andrea Santilli 2008-10-12 22:29:41 CEST
Created attachment 1886 
patch for thunar - update 9

All fixed.
Please also notice that the patch keeps the old behavior about the sidebar, hence the desktop directory won't appear there until the actual directory is created.
Comment 39 Christian Dywan 2008-10-13 18:11:42 CEST
(In reply to comment #38)
> Created an attachment (id=1886) [details]
> patch for thunar - update 9
> 
> All fixed.
> Please also notice that the patch keeps the old behavior about the sidebar,
> hence the desktop directory won't appear there until the actual directory is
> created.

Just tested, it works quite nicely with regard to showing only folders I have, thanks for your addressing my comments.

One last thing: With this version Desktop is not shown if it wasn't defined, like all other folders except Templates. The rule should probably be to hardcode ~/Desktop if it isn't set and only hide it if it's actually $HOME.
On the other hand, many people found it annoying that Thunar used to created ~/Desktop no matter what. So it might be a good thing to stop doing that now. I'm not sure, personally I'm fine with properly setting it to $HOME.
Comment 40 Andrea Santilli 2008-10-15 02:56:44 CEST
Created attachment 1893 
patch against xfdesktop svn rev 28254

Sorry, when the templates dir pointed to $HOME, both xfdesktop and thunar looked for desktop entries in the home, without really disabling it.
A normal user could get to wait for ages just not to have nothing shown.

With this patch against xfdesktop, the menu will only show the "empty file" entry when the templates dir points to $HOME
Comment 41 Andrea Santilli 2008-10-15 03:01:25 CEST
Created attachment 1894 
patch for thunar - update 10

This is the updated patch to get thunar disable the template entries too.
I guess I finally got the whole patch right.

> On the other hand, many people found it annoying that Thunar used to created
~/Desktop no matter what.

Hmm thunar creates the desktop dir only when you try to open it. It's probably xfdesktop that creates it before thunar. And yes, if the desktop dir isnt set they both default to ~/Desktop
Comment 42 Christian Dywan 2008-10-15 15:55:26 CEST
(In reply to comment #41)
> Created an attachment (id=1894) [details]
> patch for thunar - update 10
> 
> This is the updated patch to get thunar disable the template entries too.
> I guess I finally got the whole patch right.
> 
> > On the other hand, many people found it annoying that Thunar used to created
> ~/Desktop no matter what.
> 
> Hmm thunar creates the desktop dir only when you try to open it. It's probably
> xfdesktop that creates it before thunar. And yes, if the desktop dir isnt set
> they both default to ~/Desktop

Oh, in that case my bad. Great work, I'm happy then ^_^
Comment 43 Steve Graham 2008-10-15 16:35:38 CEST
*** Bug 4191 has been marked as a duplicate of this bug. ***
Comment 44 Brian J. Tarricone (not reading bugmail) 2008-10-29 09:00:33 CET
Whew, ok, that took a while to get going.

A future note: when doing development, always compiled with --enable-debug=full.  This adds '-Wall -Werror' to the gcc command line, which causes gcc to throw an error even on warnings.  Eliminate them.  Warnings are bad.

There were a few cosmetic things I changed:

1.  I didn't like how, in the Go menu, the entries show both the "friendly" name and folder name if they're different -- like in my case, "Download (downloads)" looked stupid.  So I removed that entirely; it'll only show the "friendly" name.

2.  I rearranged the order of items in the Go menu.

3.  I added correct icons to thunar-stock.[ch] for the various menu items, taken from the icon-naming spec[1] -- having 'folder' icons for all of them looks bad and is pointless.  (Better to have no icon at all than the same one over and over.)

4.  Your coding style did not match Benny's.  I fixed what I could.  Please pay closer attention next time.  In particular note Benny's brace indentation style.  Also, when breaking up statements across multiple lines, line them up at parens, e.g.:

foo = this_is_a_really_long_function_name (arg1, arg2,
                                           arg3, arg4);

... and *never* split lines between the function call name and the opening paren.

5.  When all you are doing is moving code around, don't reformat it!

Frankly, I wasted way too much time reformatting your code.  You REALLY need to pay better attention to coding style rules.  I fixed it myself this time because this patch needs to go in before this weekend and I don't have time for more back-and-forth.  But in the future, you'll be expected to get this right.

Patch-wise, there were some issues I fixed:

1.  I moved the xdg_dir_values array to thunar-shortcuts-model.c.  Otherwise there's an extra copy of this array in *every* file that includes thunar-private.h.  Not only is that a waste, it causes compiler errors.

2.  Renamed a few functions to be namespaced; prepend '_' to private functions.

3.  The special_dir_menu_entry_setup() function (renamed to something that makes more sense) was an unreadable mess of ifdefs.  I simplified it greatly and made it more readable.

4.  This is part style and part maintainability: when using a variable only inside a loop or if/then branch, declare it inside that loop or branch.  Not only does it logically group things where they are used, but it helps avoid uninitialised variable errors and things like that.

5.  Don't use thunar_vfs_path_dup_string() when a static buffer and thunar_vfs_path_to_string() will work just fine.  It's much faster and cheaper.

6.  Don't use g_strjoin() with a NULL join parameter.  That's what g_strconcat() is for (tho in the particular case I found it, g_strdup_printf() was better).

7.  Don't use g_strcmp0().  It's deprecated.  Especially never use it for utf8 strings!  Use g_utf8_collate().

8.  This bit of code was slightly entertaining:

locale = NULL;
g_free (locale);

That's not quite all that useful ^_~... and of course leaks memory.

9.  New strings that are shown in the UI need to be marked with _() for translation.  *Never* use g_strconcat() for something like that when you need to put a string variable inside a static string.  Always use g_strdup_printf(), and the translators won't hate you.


There's probably more... I know I ended up rewriting a couple functions here and there... but I need to go to bed, so I checked it in.


[1] http://standards.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html
Comment 45 Brian J. Tarricone (not reading bugmail) 2008-10-29 09:27:41 CET
Checked in new xfdesktop patch too.
Comment 46 Christian Dywan 2008-10-29 10:13:59 CET
(In reply to comment #44)
> Whew, ok, that took a while to get going.
> [...]
> 7.  Don't use g_strcmp0().  It's deprecated.  Especially never use it for utf8
> strings!  Use g_utf8_collate().

Actually quite the opposite is true, g_strcmp0 was introduced as part of the Glib test suite in Glib 2.16, so it's actually much too new. Granted, it may be the wrong choice for sorting utf8 strings.
Comment 47 Andrea Santilli 2008-10-31 22:13:50 CET
> A future note: when doing development, always compiled with
> --enable-debug=full.  This adds '-Wall -Werror' to the gcc command line, which
> causes gcc to throw an error even on warnings.  Eliminate them.  Warnings are
> bad. [..]
> 4.  Your coding style did not match Benny's.  I fixed what I could.  Please pay
> closer attention next time. [..]
> 5.  When all you are doing is moving code around, don't reformat it!

Ok, sorry. I'll pay closer attention in the future

> 8.  This bit of code was slightly entertaining:
>
> locale = NULL;
> g_free (locale);

Oops :P

> Checked in new xfdesktop patch too.

Thanks :)

It's all cool! I also like the idea of having a specific icon for each entry.

There is just a problem now though: the menu entries for the user directories don't appear if those directories are symlinks. I guess that might be useful to have them.
Comment 48 Andrea Santilli 2008-10-31 23:15:15 CET
> There is just a problem now though: the menu entries for the user directories
> don't appear if those directories are symlinks. I guess that might be useful to
> have them.

Sorry, forget about that. Some other program must have changed my configuration, that's why it wasn't working properly here.

It's perfect :)

Bug #4365

Reported by:
Andrea Santilli
Reported on: 2008-09-08
Last modified on: 2009-07-17
Duplicates (2):
  • 4062 xfdesktop doesn't obey fd.o variable XDG_DESKTOP_DIR
  • 4191 Thunar has "~/Desktop" hard-wired for the desktop directory

People

Assignee:
Jannis Pohlmann
CC List:
4 users

Version

Version:
unspecified

Attachments

patch for libxfce4util (12.32 KB, patch)
2008-09-08 09:04 CEST , Andrea Santilli
no flags
patch for thunar (28.05 KB, patch)
2008-09-08 09:05 CEST , Andrea Santilli
no flags
patch for xfdesktop (2.87 KB, patch)
2008-09-08 09:05 CEST , Andrea Santilli
no flags
updated patch for libxfce4util (10.67 KB, patch)
2008-09-13 09:04 CEST , Andrea Santilli
no flags
updated patch for thunar (30.16 KB, patch)
2008-09-13 09:04 CEST , Andrea Santilli
no flags
patch for thunar - update 2 (35.58 KB, patch)
2008-09-16 04:27 CEST , Andrea Santilli
no flags
patch for xfdesktop - update 2 (4.88 KB, patch)
2008-09-16 04:28 CEST , Andrea Santilli
no flags
patch for thunar - update 3 (32.69 KB, patch)
2008-09-20 06:30 CEST , Andrea Santilli
no flags
patch for thunar - update 4 (31.25 KB, patch)
2008-10-09 10:47 CEST , Andrea Santilli
no flags
patch for thunar - update 5 (31.24 KB, patch)
2008-10-09 19:07 CEST , Andrea Santilli
no flags
patch for thunar - update 6 (36.22 KB, patch)
2008-10-11 20:34 CEST , Andrea Santilli
no flags
patch for thunar - update 7 (36.14 KB, patch)
2008-10-11 21:46 CEST , Andrea Santilli
no flags
patch for thunar - update 8 (37.63 KB, patch)
2008-10-12 11:02 CEST , Andrea Santilli
no flags
patch for thunar - update 9 (38.34 KB, patch)
2008-10-12 22:29 CEST , Andrea Santilli
no flags
patch against xfdesktop svn rev 28254 (1.99 KB, patch)
2008-10-15 02:56 CEST , Andrea Santilli
no flags
patch for thunar - update 10 (39.28 KB, patch)
2008-10-15 03:01 CEST , Andrea Santilli
no flags

Additional information