GNOME Bugzilla – Bug 666831
Support URI opening on W32
Last modified: 2015-06-05 21:59:05 UTC
Right now several GIO functions are unimplemented on W32.
Created attachment 204195 [details] [review] Implements g_app_info_get_default_for_uri_scheme for W32
Created attachment 204196 [details] [review] Support opening URIs on W32
Review of attachment 204195 [details] [review]: ::: gio/gwin32appinfo.c @@ +656,3 @@ + * + * Returns: (transfer full): #GAppInfo for given @uri_scheme or %NULL on error. + */ Please don't add gtk-doc for these functions, they are already documented in the gdesktopfile.c version. Having the same docs in multiple places will only lead to confusion and divergence. @@ +674,3 @@ + NULL, + buffer, + &buffer_size) == S_OK) Does this return an association for e.g. file:// uris? Because anything returned here would completely override the content-type handlers that are used for the particular file.
Review of attachment 204196 [details] [review]: ::: gio/gwin32appinfo.c @@ +337,3 @@ g_win32_app_info_supports_uris (GAppInfo *appinfo) { + return TRUE; This means that all applications on windows supports opening files via a uri, as opposed to by filename. Is that really right?
(In reply to comment #3) > Review of attachment 204195 [details] [review]: > @@ +674,3 @@ > + NULL, > + buffer, > + &buffer_size) == S_OK) > > Does this return an association for e.g. file:// uris? Because anything > returned here would completely override the content-type handlers that are used > for the particular file. Yes, it would return an association for any URIs, including the "file://" one. "file://" is registered to a generic thing called "FileMoniker" in all W versions that i have access to (6.1.7601 and 5.2.3790), which proceeds to invoke the file-extension association. I wouldn't worry about content-type handlers, because get_content_type() for W boils down to g_content_type_guess(), which mostly returns file-extension anyway. (In reply to comment #4) > Review of attachment 204196 [details] [review]: > > ::: gio/gwin32appinfo.c > @@ +337,3 @@ > g_win32_app_info_supports_uris (GAppInfo *appinfo) > { > + return TRUE; > > This means that all applications on windows supports opening files via a uri, > as opposed to by filename. Is that really right? H-m-m-m...that's a tought call. I don't know of a W32 registry settings that indicates that a particular application supports URIs. In theory, you can list through all registered classes, and find the ones that have a "URL Protocol" entry (without any value), then find if a particular app is registered to any of these. If it is, it PROBABLY supports URIs (at least - URIs of this particular kind). In practice that's a LOT of keys to list through, and "URL Protocol" key may not be present in some malformed registry keys.
I don't think making everything launch through the FileMoniker is a good idea, as its a separate codepath than the other API calls and may behave differently than querying for the default AppInfo for a content type and launching that. Its just not how the APIs are meant to work. If you have a uri handler for a uri type that completely subsumes any per-file handling and only ever handle one app for that uri type. This makes sense for e.g. mailto: uris, and http: links, but not for things like file:, smb: etc. Its not a tough call. Its likely that a lot of apps don't support uris, and a change like this would make it impossible to pass file arguments to them, so its clearly wrong as is.
Created attachment 210936 [details] A simple test app that finds registered URL Protocols
Created attachment 210937 [details] A simple test app that finds registered URL Protocols (fixed access rights)
This is what i get when i run 210937: Listing 50 Root 50 has 77 subkeys, the longest subkey name of 22 wchars and class name of 0 wchars Has a proper entry: 50\BC Has a proper entry: 50\BCTP Has a proper entry: 50\DHT Has a proper entry: 50\FirefoxURL Has a proper entry: 50\ftp Has a proper entry: 50\http Has a proper entry: 50\https Has a proper entry: 50\Magnet Has a proper entry: 50\mailto Has a proper entry: 50\Thunderbird.Url.mailto Listing 58 Root 58 has 4296 subkeys, the longest subkey name of 70 wchars and class name of 0 wchars Has a proper entry: 58\aim Doesn't have a proper entry, but may be URI: 58\BC Doesn't have a proper entry, but may be URI: 58\BCTP Has a proper entry: 58\ctsu Doesn't have a proper entry, but may be URI: 58\DHT Has a proper entry: 58\Explorer.AssocProtocol.search-ms Has a proper entry: 58\file Has a proper entry: 58\ftp Has a proper entry: 58\http Has a proper entry: 58\https Has a proper entry: 58\IE.FTP Has a proper entry: 58\IE.HTTP Has a proper entry: 58\IE.HTTPS Has a proper entry: 58\iehistory Has a proper entry: 58\ierss Has a proper entry: 58\LDAP Doesn't have a proper entry, but may be URI: 58\Magnet Has a proper entry: 58\mapi Has a proper entry: 58\msnim Has a proper entry: 58\myim Has a proper entry: 58\rlogin Has a proper entry: 58\search Has a proper entry: 58\search-ms Has a proper entry: 58\StickyNotes Has a proper entry: 58\telnet Has a proper entry: 58\tn3270 Doesn't have a proper entry, but may be URI: 58\X509Enrollment.CX509PolicyServerUrl Doesn't have a proper entry, but may be URI: 58\X509Enrollment.CX509PolicyServerUrl.1 Has a proper entry: 58\xmpp Has a proper entry: 58\ymsgr Listing 80000000 Root 80000000 has 4337 subkeys, the longest subkey name of 70 wchars and class name of 0 wchars Has a proper entry: 80000000\aim Has a proper entry: 80000000\BC Has a proper entry: 80000000\BCTP Has a proper entry: 80000000\ctsu Has a proper entry: 80000000\DHT Has a proper entry: 80000000\Explorer.AssocProtocol.search-ms Has a proper entry: 80000000\file Has a proper entry: 80000000\FirefoxURL Has a proper entry: 80000000\ftp Has a proper entry: 80000000\http Has a proper entry: 80000000\https Has a proper entry: 80000000\IE.FTP Has a proper entry: 80000000\IE.HTTP Has a proper entry: 80000000\IE.HTTPS Has a proper entry: 80000000\iehistory Has a proper entry: 80000000\ierss Has a proper entry: 80000000\LDAP Has a proper entry: 80000000\Magnet Has a proper entry: 80000000\mailto Has a proper entry: 80000000\mapi Has a proper entry: 80000000\msnim Has a proper entry: 80000000\myim Has a proper entry: 80000000\rlogin Has a proper entry: 80000000\search Has a proper entry: 80000000\search-ms Has a proper entry: 80000000\StickyNotes Has a proper entry: 80000000\telnet Has a proper entry: 80000000\Thunderbird.Url.mailto Has a proper entry: 80000000\tn3270 Doesn't have a proper entry, but may be URI: 80000000\X509Enrollment.CX509PolicyServerUrl Doesn't have a proper entry, but may be URI: 80000000\X509Enrollment.CX509PolicyServerUrl.1 Has a proper entry: 80000000\xmpp Has a proper entry: 80000000\ymsgr After obtaining this information glib can parse these keys to learn about handlers for these URIs (handlers are often stored in the registry as commandlines to run, not sure what is the best way to obtain a handler identity based on its command line...probably get an ID of the file that is being run?) and put them into a list. Then it should be possible to match any app against this list to infer its ability to handle URIs. Registry could also be monitored for changes with RegNotifyChangeKeyValue(), so the whole scanning thing can be done once (at startup or at first relevant function call), and then only updated as needed (you we can just assume that registry doesn't change that often, and mandate app restart). That still won't give accurate information about applications that ARE able to take URIs, but don't advertise that ability in any way.
Created attachment 216883 [details] [review] Support opening URIs on W32 (continued) Apply on top of attachment 204196 [details] [review] I've also discovered that _supports_uris () is NOT called in my circumstances at this moment for GLib master and a recent version of GTK2. Not sure whether that was the case earlier, when i proposed initial patch. Still, this patch addresses that problem.
Review of attachment 216883 [details] [review]: Just had a quick look, will need to do some deeper testing on win32 later. Also, you still need to make it not return a url handler for e.g. file, as that will completely override all this code anyway by making everything spawn via the FileMoniker. ::: gio/gwin32appinfo.c @@ +115,3 @@ + lresult = RegQueryInfoKey (root, NULL, NULL, NULL, &subkeys, &longest_subkey, &longest_class, NULL, NULL, NULL, NULL, NULL); + if (ERROR_SUCCESS != lresult) + { All the indentation is wrong (not glib style) @@ +116,3 @@ + if (ERROR_SUCCESS != lresult) + { + printf ("Failed to query info from %p: %lu\n", root, GetLastError ()); Don't printf (here and later), at the very most use g_warning or something that can be redirected. @@ +119,3 @@ + return result; + } + keybuf = malloc (sizeof (wchar_t) * (longest_subkey + 1)); g_malloc/g_free/g_realloc (here and later) @@ +145,3 @@ + lresult2 = RegQueryValueExW (subkey, L"URL Protocol", NULL, &valtype, NULL, NULL); + if (lresult2 == ERROR_SUCCESS && valtype == REG_SZ) + success = TRUE; Why do you need a success variable? Its just read here. @@ +195,3 @@ + url = &result[*result_len]; + *result_len += 1; + url->subkey = wcsdup (keybuf); Don't use these, use something that will cause a g_malloc rather than a malloc. @@ +286,3 @@ #endif + + for (i = 0; i < urls_len && !info->supports_uris; i++) This seems pretty costly to do every time, for every url handler. can't you store the info in a hashtable to that the lookup is faster?
(In reply to comment #11) > Review of attachment 216883 [details] [review]: > > Just had a quick look, will need to do some deeper testing on win32 later. > > Also, you still need to make it not return a url handler for e.g. file, as that > will completely override all this code anyway by making everything spawn via > the FileMoniker. Yeah, i remembered that point and tried a few file:/// links as well. They worked correctly (i.e. results were not unexpected) from my point of view. To fix this (if it actually requires fixing) i have to understand what exactly is wrong with using FileMoniker for file:/// uris. > ::: gio/gwin32appinfo.c > @@ +115,3 @@ > + lresult = RegQueryInfoKey (root, NULL, NULL, NULL, &subkeys, > &longest_subkey, &longest_class, NULL, NULL, NULL, NULL, NULL); > + if (ERROR_SUCCESS != lresult) > + { > All the indentation is wrong (not glib style) That is easily fixable, and is not my primary concern. > @@ +116,3 @@ > + if (ERROR_SUCCESS != lresult) > + { > + printf ("Failed to query info from %p: %lu\n", root, GetLastError ()); > Don't printf (here and later), at the very most use g_warning or something that > can be redirected. Sorry, leftovers from my testapp. Had more printfs, removed almost all of them, but forgot to convert the remaining one(s) to g_warning(). > @@ +119,3 @@ > + return result; > + } > + keybuf = malloc (sizeof (wchar_t) * (longest_subkey + 1)); > g_malloc/g_free/g_realloc (here and later) Is there a point in using Glib memory allocators in cases where pointers to allocated memory are not passed anywhere? OK, i'll do s/malloc/g_malloc/ s/free/g_free/ s/realloc/g_realloc/. > @@ +145,3 @@ > + lresult2 = RegQueryValueExW (subkey, L"URL Protocol", NULL, &valtype, > NULL, NULL); > + if (lresult2 == ERROR_SUCCESS && valtype == REG_SZ) > + success = TRUE; > > Why do you need a success variable? Its just read here. Leftover from the old code (success was required back then). > @@ +195,3 @@ > + url = &result[*result_len]; > + *result_len += 1; > + url->subkey = wcsdup (keybuf); > > Don't use these, use something that will cause a g_malloc rather than a malloc. That's actually a good point. > @@ +286,3 @@ > #endif > + > + for (i = 0; i < urls_len && !info->supports_uris; i++) > > This seems pretty costly to do every time, for every url handler. can't you > store the info in a hashtable to that the lookup is faster? Number of url handlers on my machine is 40, and the matching being done is not trivial in some cases (as you'll see a few lines below). Maintaining a hashtable takes more code and care, right now i'm more interested in figuring out the right logic rather than making things work at optimal speed.
> To fix this (if it actually requires fixing) i have to understand what exactly > is wrong with using FileMoniker for file:/// uris. Any code that has a filename (i.e. file: GFile) and want to open it with the app registered for that type will call g_file_query_default_handler(). As long as we return the FileMoniker for file: files this means we will always return the FileMoniker AppInfo, so we will never actually even call g_app_info_get_default_for_type() and find the "real" AppInfo that represents the actual app that we will launch. So, we'll show "File moniker" in the open-with menu for all local files. We'll only report launch errors from spawning the FileMoniker in the GError in g_app_info_launch. We will never return the right value for "supports uris" (only ever what the filemoniker supports). It will be harder to support more complex things in the GAppLaunchContext. And just in general we'd have two codepaths for the same operation depending on how you got the GAppInfo handler for the file, which is never good. We have historically had a lot of problems with situations just like this, which is why I bring it up.
> Is there a point in using Glib memory allocators in cases where pointers to > allocated memory are not passed anywhere? OK, i'll do s/malloc/g_malloc/ > s/free/g_free/ s/realloc/g_realloc/. malloc may return NULL, wheras g_malloc exits on OOM. Also, you can use the glib api to change what g_malloc does, which we want to support for all allocations done by glib.
Created attachment 283572 [details] A simple test app that finds registered URL Protocol handlers Uses the API from bug 734888.
Created attachment 283573 [details] Output from the attachment 283572 [details]
The attachment 283572 [details] walks HKLM/Software up to 4 levels deep. This takes a while (a few seconds). While this approach works (and is simple, with the walker API available), it should be much faster to only *list* HKLM/Software/RegisteredApplications, which contains a list of values that point to the keys that need to be read, and to walk HKLM/Software/Clients (which is much smaller than the whole HKLM/Software).
Created attachment 284856 [details] [review] GWin32AppInfo rewrite - On first call scan the registry, collect information about URI protocols, file extensions, applications and handlers, store that as a set of interconnected structures in several hash tables - Watch the registry keys, re-scan the registry when any one of them changes.
With attachment 284856 [details] [review] i was able to click on a link in About dialog, and it opened Firefox! Which was the point all along.
Code is somewhat...hairy. Some duplication is present. However, i'm not sure about stuffing duplicated parts into functions. I imagine how the code would look then, and i don't like the image that comes up in my mind.
Review of attachment 284856 [details] [review]: See the inline comments. As you can see they are nitpicks. I think Alex should be the one doing the real review here. Would be great though to have some unit tests. ::: gio/gwin32appinfo.c @@ +3,2 @@ * Copyright (C) 2006-2007 Red Hat, Inc. + * Copyright (C) 2014 LRN name @@ +18,3 @@ * + * Authors: Alexander Larsson <alexl@redhat.com> + * LRN <lrn1986@gmail.com> name @@ +297,3 @@ + g_clear_object (&handler->proxy_key); + g_clear_object (&handler->icon); + g_clear_pointer (&handler->proxy_command, g_free); missing chain up @@ +309,3 @@ + g_clear_object (&ext->chosen_handler); + g_clear_pointer (&ext->handlers, g_hash_table_destroy); + g_clear_pointer (&ext->other_apps, g_hash_table_destroy); also here missing chain up @@ +332,3 @@ + g_clear_pointer (&app->supported_urls, g_hash_table_destroy); + g_clear_pointer (&app->supported_exts, g_hash_table_destroy); + g_clear_pointer (&app->command, g_free); and here @@ +439,2 @@ +/* Watch this whole subtree */ +GWin32RegistryKey *url_associations_key; there are several vars here that aren't static, shouldn't be? @@ +564,3 @@ + gint counter; + GWin32RegistryKey *key; + *icon_out = NULL; leave empty line before this assign @@ +604,3 @@ + + if (*icon_out) + TRUE, I'd rather have a break here. @@ +654,3 @@ + if (key != NULL) + { + ok = g_win32_registry_key_get_value_w (key, I don't like the name ok for a var ::: gio/gwin32appinfo.h @@ +3,2 @@ * Copyright (C) 2006-2007 Red Hat, Inc. + * Copyright (C) 2014 LRN Use your name @@ +18,3 @@ * + * Authors: Alexander Larsson <alexl@redhat.com> + * LRN <lrn1986@gmail.com> same here
Created attachment 300992 [details] [review] GWin32AppInfo rewrite * Changed my name in file headers. * Updated the patch to apply on top of current git HEAD * Updated the patch to apply on top of latest version of the patch from bug 734888 * Some global vars are now static. * Chained up dispose functions * Changed ok -> got_value * return -> break in read_handler_icon() * Added link_handlers_to_unregistered_apps() post-processing function, now applications only known by their executable basename can be found by a link from handlers. * Fixed a leak with the classes_root variable. * Codestyle fixes Note: the patch looks bizarre, because it tramples some of the recent changes. It is a ~70% rewrite, so it's better to view it with git diff -B.
Honestly, you guys know far more about the win32 details than I atm. I don't even have a win32 machine set up for building glib atm. So, I can only give some overview feedback. First of all, I think a lot of this patch would be much more readable if you added a va_list based helper to build registry paths, similar to g_strconcat. Something like gboolean build_registry_key (gunichar2 *buffer, ...); and then you can do if (!build_registry_key (path, FILE_EXT, ext, USER_CHOICE, NULL)) return; user_choice = g_win32_registry_key_new_w (path, NULL); rather than: if ((sizeof (FILE_EXTS) + wcslen (ext) * sizeof (gunichar2) + sizeof (USER_CHOICE) return; path[0] = L'\0'; p = path; wcscat (p, FILE_EXTS); p += wcslen (FILE_EXTS); wcscat (p, ext); p += wcslen (ext); wcscat (p, USER_CHOICE); user_choice = g_win32_registry_key_new_w (path, NULL); In a lot of places. I don't understand why you are using g_utf8_collate_key() on a bunch of strings. I mean, i understand the case folding, but isn't collation mainly for ordering strings "alphabetically"? The handling of launching seems a bit unixy. Why are you setting DISPLAY? And the startup notification seems like dead code that can't be triggered, and that won't work anyway, because no win32 apps look at the DESKTOP_STARTUP_ID env var. Maybe we can do a real win32 startup notification thing that returns that the app is launched when the first window is mapped with some known window class. And finally, I repeat my above worries that you're listing a "file:" uri handler by default. This means, for instance that g_file_query_default_handler() will return the file moniker as the default app for every file, independent of its actual file type. I realize that this will still let you *launch* the app. But it will not pick up the right name for displaying in e.g. the open with menu entry for the file, also it will return the wrong info for e.g. g_win32_app_info_supports_uris() on the app-info that the app got from g_file_query_default_handler(), as it will always be based on the file moniker details. We used to have a lot of issues like this in gnome-vfs where we had a similar lower level handler for mime types, and then a higher-level "entire url scheme" handler. Some ftp app would claim the entire ftp scheme, and then when browsing a ftp directory in nautilus and clicking on a pdf file it would not launch the pdf reader as expected due to the mimetype, but instead a ftp app.
(In reply to Alexander Larsson from comment #23) > First of all, I think a lot of this patch would be much more readable if you > added a va_list based helper to build registry paths, similar to g_strconcat. > Something like > gboolean build_registry_key (gunichar2 *buffer, ...); > > and then you can do > > if (!build_registry_key (path, FILE_EXT, ext, USER_CHOICE, NULL)) > return; > user_choice = g_win32_registry_key_new_w (path, NULL); > > rather than: > > if ((sizeof (FILE_EXTS) + > wcslen (ext) * sizeof (gunichar2) + > sizeof (USER_CHOICE) > return; > path[0] = L'\0'; > p = path; > wcscat (p, FILE_EXTS); > p += wcslen (FILE_EXTS); > wcscat (p, ext); > p += wcslen (ext); > wcscat (p, USER_CHOICE); > user_choice = g_win32_registry_key_new_w (path, NULL); > > In a lot of places. Noted. Should this be a part of the registry API (see the other bug report), or just a static func here? > I don't understand why you are using g_utf8_collate_key() on a bunch of > strings. I mean, i understand the case folding, but isn't collation mainly > for ordering strings "alphabetically"? This code: > x = g_utf8_collate_key (X); > y = g_utf8_collate_key (Y); > result = strcmp (x, y) is supposed to be an equivalent of: > result = g_utf8_collate (x, y); preemptive separate collation of strings is to account for the fact that hash table uses some kind of strcmp for key equality checks. At least that's what i've read. > The handling of launching seems a bit unixy. Why are you setting DISPLAY? > And the startup notification seems like dead code that can't be triggered, > and that won't work anyway, because no win32 apps look at the > DESKTOP_STARTUP_ID env var. Maybe we can do a real win32 startup > notification thing that returns that the app is launched when the first > window is mapped with some known window class. There may be some unixisms left over from the code that i borrowed from the unixy implementation of launching. If anyone can point them out in detail, i should be able to fix them. What is "startup notification"? Should it work for any program or just for GUI ones? For any program or just for the ones that are glib-based? How is it used in practice? > And finally, I repeat my above worries that you're listing a "file:" uri > handler by default. This means, for instance that > g_file_query_default_handler() will return the file moniker as the default > app for every file, independent of its actual file type. I realize that this > will still let you *launch* the app. But it will not pick up the right name > for displaying in e.g. the open with menu entry for the file, also it will > return the wrong info for e.g. g_win32_app_info_supports_uris() on the > app-info that the app got from g_file_query_default_handler(), as it will > always be based on the file moniker details. > > We used to have a lot of issues like this in gnome-vfs where we had a > similar lower level handler for mime types, and then a higher-level "entire > url scheme" handler. Some ftp app would claim the entire ftp scheme, and > then when browsing a ftp directory in nautilus and clicking on a pdf file it > would not launch the pdf reader as expected due to the mimetype, but instead > a ftp app. What can be done about this? Maintain a list of known "overgeneric" handlers and demote or delist them?
> Noted. Should this be a part of the registry API (see the other bug report), > or just a static func here? It seems a bit specific. Make it a static for now. > This code: > > > x = g_utf8_collate_key (X); > > y = g_utf8_collate_key (Y); > > result = strcmp (x, y) > > is supposed to be an equivalent of: > > > result = g_utf8_collate (x, y); > > preemptive separate collation of strings is to account for the fact that hash > table uses some kind of strcmp for key equality checks. At least that's what > i've read. This is a bit confused. You do case folding because you want "foo", "Foo", and "FOO" to all compare equal. I understand that. However, collation is about localized alphabethical order of string. I.f. if you call g_list_sort(list, strcmp) you will get a sort on byte values, but if you do g_list_sort(list, g_utf8_collate) you will get a localized sort. However, this comparison function is quite inefficient, so on can call g_utf8_collate_key() on all the elements in the list, then one can use a regular g_strcmp comparison function to get a localized order. The advantage here is efficiency, since we compare using the same keys multiple times but need to convert to collation keys only once. However, this is all irrelevant here, because we're using a hashtable. The hashtable is using strcmp for comparisons, yes, but it is *only* checking if it returns 0 or not, as its not relying on the *order* of strings. This means it is safe to use the regular strcmp without collation keys. > There may be some unixisms left over from the code that i borrowed from the > unixy implementation of launching. If anyone can point them out in detail, i > should be able to fix them. The setting of DISPLAY and DESKTOP_STARTUP_ID just are not useful on win32. > What is "startup notification"? Should it work for any program or just for > GUI ones? For any program or just for the ones that are glib-based? How is it > used in practice? startup notification is an optional protocol that lets the launcher of an application pass some launch information to the launched app and to get told by the application when the app is "ready" (typically when the first window is displayed). This typically show to show a spinning cursor and some placeholder text/icon in the task bar while the app is starting. It is also a place where the launching app can put the timestamp of the event that caused the app to be launched, which is then used by the focus-stealing prevention mechanism in the desktop. I.e. if you did any input in another window *after* the time of the event that launched the app then we will not give focus to the new window when it is presented. On unix this is triggered by the StartupNotify=true in the desktop file, which is opt-in (if the app implements the right freedesktop specs), as we can only use this if the app supports it. I think we can probable ignore this on win32. I don't think there is a similar native protocol we could use. > What can be done about this? Maintain a list of known "overgeneric" handlers > and demote or delist them? Just black-listing "file" scheme is probably enough.
(In reply to Alexander Larsson from comment #25) >> This code: >> >> > x = g_utf8_collate_key (X); >> > y = g_utf8_collate_key (Y); >> > result = strcmp (x, y) >> >> is supposed to be an equivalent of: >> >> > result = g_utf8_collate (x, y); >> >> preemptive separate collation of strings is to account for the fact that hash >> table uses some kind of strcmp for key equality checks. At least that's what >> i've read. > > This is a bit confused. You do case folding because you want "foo", "Foo", > and "FOO" to all compare equal. I understand that. However, collation is > about localized alphabethical order of string. > > I.f. if you call g_list_sort(list, strcmp) you will get a sort on byte > values, but if you do g_list_sort(list, g_utf8_collate) you will get a > localized sort. However, this comparison function is quite inefficient, so > on can call g_utf8_collate_key() on all the elements in the list, then one > can use a regular g_strcmp comparison function to get a localized order. The > advantage here is efficiency, since we compare using the same keys multiple > times but need to convert to collation keys only once. > > However, this is all irrelevant here, because we're using a hashtable. The > hashtable is using strcmp for comparisons, yes, but it is *only* checking if > it returns 0 or not, as its not relying on the *order* of strings. This > means it is safe to use the regular strcmp without collation keys. Okay, i'll remove collation.
Created attachment 301760 [details] [review] GWin32AppInfo rewrite * Added build_registry_path() * Converted all uses of wcscat() to build_registry_path() * Removed collation of strings * Added optional utf8_and_fold() call to follow_class_chain_to_handler(), since follow_class_chain_to_handler() was almost always followed by utf8_and_fold() anyway. * Blacklisted "file:" URI: * g_win32_app_supports_uris() needs to find support for something other than "file:" to return TRUE * g_app_info_get_default_for_uri_scheme() returns NULL for the "file:" scheme * More indentation fixes * Added partial support for App Paths: now launcher can prepend extra paths when launching certain programs.
Review of attachment 301760 [details] [review]: Pointed out some details below, but overall I can't really verify how right this is from a win32 API point of view, as I don't have a win32 build here atm, nor do i remember how much of this stuff works. However, it seems OK from what I can see, and I don't see how this now could really break anything. So assuming that there has been some level of testing of this on win32 so that it actually works, I'd say this is ready to go in. If some detail needs fixing we can do that later. ::: gio/gwin32appinfo.c @@ +641,3 @@ + return FALSE; + + path = g_malloc (sizeof (gunichar2) * (length + 1)); Since you're mainly using this for temporary strings, and it has a fixed size, why not pass in a fixed size buffer from the caller instead and avoid the malloc (and the related free in all callers). @@ +733,3 @@ + return FALSE; + + key = g_win32_registry_key_new_w (reg_path, NULL); build_registry_path() + g_win32_registry_key_new_w() seems like a common operation. Add helper for this? @@ +4266,3 @@ + break; + } + } Eh, why are you looping over all the keys and comparing? Can't you just do a lookup in the hash table? @@ +4497,3 @@ + scheme->chosen_handler != NULL && + scheme->chosen_handler->app != NULL && + strcmp (scheme->schema_folded, "file") != 0) Can't you just exist early if the passed in uri_scheme is "file"?
(In reply to Alexander Larsson from comment #28) > Review of attachment 301760 [details] [review] [review]: > > Pointed out some details below, but overall I can't really verify how right > this is from a win32 API point of view, as I don't have a win32 build here > atm, nor do i remember how much of this stuff works. However, it seems OK > from what I can see, and I don't see how this now could really break > anything. So assuming that there has been some level of testing of this on > win32 so that it actually works, I'd say this is ready to go in. If some > detail needs fixing we can do that later. I've done some limited testing. If you know any apps that make use of all AppInfo functionality, i'd be happy to test there. > > ::: gio/gwin32appinfo.c > @@ +641,3 @@ > + return FALSE; > + > + path = g_malloc (sizeof (gunichar2) * (length + 1)); > > Since you're mainly using this for temporary strings, and it has a fixed > size, why not pass in a fixed size buffer from the caller instead and avoid > the malloc (and the related free in all callers). Makes sense. > > @@ +733,3 @@ > + return FALSE; > + > + key = g_win32_registry_key_new_w (reg_path, NULL); > > build_registry_path() + g_win32_registry_key_new_w() seems like a common > operation. Add helper for this? Yes, makes sense. There's only one place where build_registry_path() result is not freed immediately after opening a registry key, so a helper would...well...help. > > @@ +4266,3 @@ > + break; > + } > + } > > Eh, why are you looping over all the keys and comparing? Can't you just do a > lookup in the hash table? Right...i wasn't thinking straight. > > @@ +4497,3 @@ > + scheme->chosen_handler != NULL && > + scheme->chosen_handler->app != NULL && > + strcmp (scheme->schema_folded, "file") != 0) > > Can't you just exist early if the passed in uri_scheme is "file"? Right. I think i can. Didn't make a connection between uri_scheme and scheme->schema_folded.
Created attachment 302659 [details] [review] GWin32AppInfo rewrite * Changed build_registry_path() to accept a (usually static) buffer and its size instead of allocating a new buffer internally. * Added build_registry_pathv() * Added _g_win32_registry_key_build_and_new_w(), which combines g_win32_registry_key_new_w() and build_registry_path() * Made the code use _g_win32_registry_key_build_and_new_w() where appropriate * Fixed a bug in appinfo acquiring a list of supported extensions (wrong order of arguments to g_hash_table_iter_next()) * Removed redundant code in g_win32_app_supports_uris(), now it does a straight "file" lookup. Clarified the code as well. * Bail out of g_app_info_get_default_for_uri_scheme("file") earlier.
Created attachment 302660 [details] A small test application for running default apps for files or URLs Compile with: gcc `pkg-config glib-2.0 --cflags` appinfo-test.c -o appinfo-test.exe `pkg-config glib-2.0 gio-2.0 --libs`
Known bug: glib always passes /-delimited filename (even if it was \\-delimited when given to GFile) in the commandline of an application that it launches. Believe it or not, there are programs that do *not* handle /-delimited filenames. I even have two programs like this. Forcing names to be \\-delimited is not the right fix for this, as there are programs that do not handle \\-delimited filenames or require escaping for backslashes (which glib can't know about).
Review of attachment 302659 [details] [review]: Hi LRN, Please see the following comment: With blessings, thank you! ::: gio/gwin32appinfo.c @@ +646,3 @@ + return FALSE; + + va_copy (lentest, components); Please use G_VA_COPY() so that it works under non-GCC as well.
Created attachment 303476 [details] [review] Make GWin32AppInfo MSVC-compatible - use G_VA_COPY
The major user of these APIs is nautilus, which is probably hard to test on win32. Did you try the GtkAppChooser widget? There is s test for it in gtk.
Review of attachment 302659 [details] [review]: Other than that this looks ok to me. I haven't really tested it as I don't have a win32 setup here. I don't want to block this more, as it does seem to be useful work for win32, and does not affect non-win32. ::: gio/gwin32appinfo.c @@ +658,3 @@ + + if ((length >= REG_PATH_MAX_SIZE) || + (length * sizeof (gunichar2) >= output_size)) I believe the later check needs to be length+1 to include termination.
Attachment 302659 [details] pushed as 4d800e4 - GWin32AppInfo rewrite Attachment 303476 [details] pushed as 2a71f18 - Make GWin32AppInfo MSVC-compatible - use G_VA_COPY