Page MenuHome

Windows Bookmarks Should Use CSIDL_MYDOCUMENTS
AbandonedPublic

Authored by Harley Acheson (harley) on Dec 30 2018, 4:03 AM.

Details

Summary

On the File Editor, when using Windows, the bookmarks are populated with the "My Documents"
and "Desktop" folders.

When finding these folders, we are using SHGetSpecialFolderPathW with a CSIDL of CSIDL_PERSONAL
for the "My Documents" folder. However as of Windows XP (at or below our oldest supported Windows
version) we are supposed to be using a CSIDL of CSIDL_MYDOCUMENTS.

This patch makes this change to use the new parameter.

In almost all cases this change will have no effect as these should give the same result. But there are
some circumstances where the user has moved their defaults "documents" location from default
and this will yield better results.

Diff Detail

Repository
rB Blender

Event Timeline

On XP and up CSIDL_PERSONAL and CSIDL_MYDOCUMENTS are equivalent and since the lowest we support is vista this change should not make a difference on any of our supported platforms.

I have nothing against this change, but given this diff changes nothing , I can't help wondering why do it at all? What problem are we solving here?

You are probably right, as I can't find an example where the two would be different. It looks like
they had different uses prior to XP, but are equivalent since.

My initial thought was that it was just best practice to use the OS's recommended method for
the OSs that we support.

But with in mind we should instead consider changing from the unsupported SHGetSpecialFolderPathW
(has trouble with some non-ASCII characters) and move to it's replacement of SHGetKnownFolderPath
(Vista and newer), although I don't like that minimum server OS for that is 2008.

In summary, this patch doesn't accomplish anything in a real sense so we can ignore it.

But with in mind we should instead consider changing from the unsupported SHGetSpecialFolderPathW

It's only unsupported in a sense of that the arm builds don't have it, and given we don't support arm on windows currently, no big deal. (currently, if we ever go the arm route this will have to change)

(has trouble with some non-ASCII characters)

citation needed, the W at the end signifies it's at unicode version of this function, why would there be trouble with non ascii chars? (I'm not saying there isn't i just like to understand a problem before we fix it)

and move to it's replacement of SHGetKnownFolderPath (Vista and newer), although I don't like that minimum server OS for that is 2008.

That's fine,Vista and 2008 share a codebase. The version before 2008 is 2003(R2) which is still based on XP which we no longer support, so this function is safe to use.

In summary, this patch doesn't accomplish anything in a real sense so we can ignore it.

Not entirely true, even a cleanup diff is welcome, but i generally try to understand why things are changed before I apply them. If the reason is cleanup, great! however if there's an actual problem you ran into that patch is solving, I'd like to know about it/understand what was wrong :)

citation needed

No need as you are right again. When I was googling about SHGetSpecialFolderPathW no longer being supported
I had found some hits that mentioned that *and* about complaints about handling of non-ascii characters when
using the non-wide version. LOL

if there's an actual problem you ran into that patch is solving

No, there really was not. I think I was bumbling around looking at how we deal with OS-specific special paths while
contemplating the possibility of getting a default in for user fontdir. So was just looking around while bored on a
Saturday evening. LOL

Nothing wrong with that, if you want to do some cleanup and replace the SHGetSpecialFolderPathW calls with SHGetKnownFolderPath I say go for it, I'll happily accept cleanup patches, it's not the most glamorous work but it's a great way to contribute some code :)

@LazyDodo (LazyDodo), I'll leave this up to you to commit / accept, if you don't mind.

I still say just close this ticket.

If I get around to swapping those calls I'll make a new ticket.

Thanks Ray!