Opened 6 years ago

Closed 4 years ago

#8874 closed enhancement (fixed)

support for windows unique volume names

Reported by: talon_karrde Owned by:
Priority: low Milestone:
Component: base Version: stable-latest
Keywords: windows unique volume name Cc: talon_karrde
Blocked By: Blocking:
Patch: yes

Description

this patch enables users to use wxFileName with paths that begin with windows unique volume names (
?\Volume{guid}\) instead of a drive letter. It partly uses the UNC paths workaround, especially in wxFileName::GetVolumeString used by wxFileName::GetPath. The problem with using this approach is that if the user wants to call wxFileName.SetVolume with a unique volume name, they must know how the underlying system works and remove the first two backslashes from the volume name themselves before passing the volume name to SetVolume.
I've also added a check in ::wxDirExists() for unique volume names so that such a name will be recognized as a directory, just as C:\ is recognized.
N.B.: I'm adding this patch in the common directory although the issues it deals with are MSW-specific because the source files it modifies are located in src/common

Attachments (2)

Windows unique volume names support.patch download (2.8 KB) - added by talon_karrde 6 years ago.
patch for unique volume names support
Windows_unique_volume_names_support.patch download (8.9 KB) - added by talon_karrde 4 years ago.

Download all attachments as: .zip

Change History (14)

Changed 6 years ago by talon_karrde

patch for unique volume names support

comment:1 Changed 6 years ago by vadz

Thanks for your patch but I believe something is missing from it as WIN_UNIQUE_VOLNAME_LAST_CHAR_INDEX used in it is not defined anywhere and so it won't compile currently -- hence it can't be applied.

I also think that IsWinUniqueVolumeNamePath() should be a static public member of wxFileName so that wxDirExists could reuse it instead of partly duplicating its logic.

But beyond this I also wonder when/why exactly would wx applications need to operate with the volume names in this format?

comment:2 Changed 6 years ago by talon_karrde

You're right, it seems I've omitted the define of WIN_UNIQUE_VOLNAME_LAST_CHAR_INDEX out of the patch, and I'll make IsWinUniqueVolumeNamePath() a static public member of wxFileName as you suggested - it makes sense. Which leaves me with the question where to put the above-mentioned constant - if it was up to me to decide I'd just make it a public static const member of wxFileName as well but I have the feeling this isn't the way you organize global constants in wxWidgets. Should I just make it a global constant like wxInvalidSize?
As for your question, I admit that it will be far from frequently used or even noticed, the reason I had to tinker at this at all is because I'm working over an app that hides some of the volumes from the windows user in order to do some neat tricks and a UI app needs those unique volume names in such cases in order to operate with the hidden volumes. Not something that you'll want to do every day but since I added such functionality anyway, I felt it would be too selfish of me not to share it if someone else by accident needed the same thing:)

comment:3 Changed 6 years ago by vadz

Does this constant have to be public at all? I don't think so and if I'm right then you could just use a static const variable in filename.cpp as usual.

And thanks for the explanation, I see it now. But could you please also add a brief comment somewhere explaining what these unique volume names are (maybe with an URL containing more information if there is one somewhere) and what are they useful for? Otherwise I imagine it could be pretty confusing to the next person (who wouldn't have folowed this patch discussion) reading this code.

Thanks in advance!

comment:4 Changed 6 years ago by sf-robot

This Tracker item was closed automatically by the system. It was
previously set to a Pending status, and the original submitter
did not respond within 14 days (the time period specified by
the administrator of this Tracker).

comment:5 Changed 5 years ago by talon_karrde

  • Component set to base
  • Keywords windows unique volume name added
  • Status changed from closed to reopened
  • Type set to enhancement
  • Version set to 2.9-svn

I've finally gotten to modify and resend the patch for this issue, sorry for the extreme delay.
This patch has been made againt the trunk, I suppose 2.9-svn is the closest value I could choose for the Version field.
Please tell me if there is still something wrong with the patch as I haven't been around for some time so there is every chance I could have gotten something wrong.
Btw is there a way to remove the old patch file from this ticket as it's not needed and can just confuse people?

comment:6 Changed 4 years ago by vadz

  • Cc vadz removed
  • Status changed from reopened to confirmed

Thanks for the update! Unfortunately the patch doesn't apply right now because of the changes in the trunk since then, could you please redo it? It shouldn't be difficult to fix it but I didn't want to do it because the other points:

  1. New function must be documented in interface/wx/filename.h.
  2. I still wonder why should this constant need to be public, does the user code really need to access it? If so, why (i.e. could you please mention what is it useful for in the documentation of this constant too)?
  3. It would be great to have unit tests for this stuff in tests/filename/filenametest.cpp because as this is something that is probably not used very often, chances of breaking support for volume names and not noticing it are very high. Could you please use them in a couple of the tests?

TIA!

comment:7 follow-up: Changed 4 years ago by talon_karrde

OK, I'll get down to doing points 1 and 3 shortly.
As for 2 - I can't think of any case in which the constant will be useful to the user right now, and I'm not sure any exists. The reason I made it public is because it's needed both in ::wxDirExists() and in wxFileName's methods, so it had to be exposed. And I declared it with an export declaration because any user code can use it once it's been declared like that, and I don't see any point in allowing users to use it only if they link statically to wxWidgets. Any suggestions are welcome.

comment:8 in reply to: ↑ 7 Changed 4 years ago by vadz

Replying to talon_karrde:

OK, I'll get down to doing points 1 and 3 shortly.

TIA!

As for 2 - I can't think of any case in which the constant will be useful to the user right now, and I'm not sure any exists. The reason I made it public is because it's needed both in ::wxDirExists() and in wxFileName's methods

The best way to fix this would be to move wxDirExists() contents to wxFileName::DirExists() and just call the latter from the former (instead of vice versa, as it is done now). This is, of course, an unrelated change and should be done as a separate patch but OTOH it does make sense as the long-term plan is to get rid of all the global file-related functions and use wxFileName for everything anyhow. If you could please do it (and probably for wxFileExists() too for consistency) it would be great.

Thanks again!

comment:9 Changed 4 years ago by talon_karrde

I submitted a patch which moves the logic from ::wxFile/DirExists() to wxFileName::File/DirExists as proposed, see http://trac.wxwidgets.org/ticket/11488.
I'll wait for that patch to be committed to trunk before creating a new patch for this ticket, because I wouldn't want to manually edit the diff that would result if I made the changes over the dirty trunk now.

Regards,
Neno Ganchev

Changed 4 years ago by talon_karrde

comment:10 Changed 4 years ago by talon_karrde

Here's the patch with added documentation and unit tests (which were actually very helpful:).

comment:11 Changed 4 years ago by vadz

Thanks, will apply soon but I'll rename the function to IsMSWUniqueVolumeNamePath() for consistency (a few of the existing MSW-only functions use "MSW" in their names and AFAIK we don't use "Win" anywhere).

comment:12 Changed 4 years ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

(In [62782]) Add support for MSW unique volume names to wxFileName.

Recognize the paths starting with "
?\Volume{GUID}" under MSW and provide a
way to test for them.

Closes #8874.

Note: See TracTickets for help on using tickets.