Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some issues and remarks on all of the proposed solutions #1

Open
guwidoe opened this issue Dec 16, 2023 · 4 comments
Open

Some issues and remarks on all of the proposed solutions #1

guwidoe opened this issue Dec 16, 2023 · 4 comments

Comments

@guwidoe
Copy link

guwidoe commented Dec 16, 2023

This will be a longer issue since I have spent hundreds of hours on this problem and am always excited to see new and creative solutions like the ones presented here.

First of all, thank you very much for sharing these solutions! I think they are very interesting and innovative approaches and I did not expect to find such novel approaches to this problem anymore, as there are already so many attempts at solving this available online.

Solution 1 (via recently used files):

This is a very creative and great solution for this problem which I haven't seen before. In my testing, it worked quite reliably, but it does have some drawbacks, one of which you already pointed out in your README.md which is that "Show Recently Opened Items" needs to be enabled.

Issue 1:

It also does not work on Workbooks that are opened with the Workbooks.Open method. Theoretically, this method offers an optional parameter that lets the user decide whether to add the opened Workbook to the list of recently used files (AddToMru), however unfortunately the default value is AddToMru:=False.
Even more unfortunately, there seems to be a bug in the Workbooks.Open method so that even if specifying AddToMru:=True, the shortcut in the recently used files list was not created in my testing, making GetThisWorkbookLocalPath1 unsuitable for Workbooks opened via this method.

Issue 2:

There seems to be an issue with the fso.FileExists() method. When I tested the function on a file with a local path like this:

C:\Users\Witt-DörringGuido\OneDrive - TU Wien\😀👩‍👩🦲👩👩‍👩‍👧‍👦🦲‍👩‍👧‍👦🦲‍👧‍👦Test𐀀😀‍👩👩‍👩‍👧‍👦💁🏼‍♀️🧔🏻‍♂️👩‍❤️‍👨🏃🏻‍♀️\😀👩‍👩🦲👩👩‍👩‍👧‍👦🦲‍👩‍👧‍👦🦲‍👧‍👦Test𐀀😀‍👩👩‍👩‍👧‍👦💁🏼‍♀️🧔🏻‍♂️👩‍❤️‍👨🏃🏻‍♀️\T

the logic worked correctly until the line If Not fso.FileExists(filePath) Then Exit Function, where even though filePath contained the correct path, the function was exited and nothing was returned.

Suggestion 1:

I think you could even consider removing this line completely, because while it is possible that filePath contains the wrong path, If Not fso.FileExists(filePath) Then Exit Function can not always prevent you from returning that wrong path.

Issue 3, (consequence of Issue 1)

Consider the following example:
You open a file called test.xlsm manually from the location C:\users\username\OneDrive\folder1\test.xlsm. There will now be a link called test.xlsm in the recent items folder.

Now you close that file, and open another file called test.xlsm but this time from a different location C:\users\username\OneDrive\folder2\test.xlsm AND by using the Workbooks.Open() method directly from VBA.

Now, running GetThisWorkbookLocalPath1 from the VBA project of that second file will return the wrong path C:\users\username\OneDrive\folder1\, because the shortcut in the recent items folder has not been overwritten due to the usage of Workbooks.Open(). Obviously in this case If Not fso.FileExists(filePath) Then Exit Function also wouldn't prevent you from returning the wrong path.

Moreover, if GetThisWorkbookLocalPath1 is used correctly, keeping in mind its restrictions which is that it does not work on Workbooks opened using the Workbooks.Open method, and if the second file would therefore have been opened manually, then the link would have been replaced and the correct path would be returned, regardless of If Not fso.FileExists(filePath) Then Exit Function.

Solution 2 (via Environ() or Shell.Application.Windows)

The solution using Environ() I have seen countless times already, and there are countless issues with it, some of which you try to overcome by using a method I haven't seen before as an alternative. But first things first:

1) Case LCase(ThisWorkbook.Path) Like "https://d.docs.live.net/????????????????"

This should work as implemented. I overlooked something in the code at first and therefore pointed out an issue that wasn't there. This case should deal with files in the personal OneDrive folder that are not located in any subfolder, which is a pretty niche use case.

2) Case LCase(ThisWorkbook.Path) Like "https://*.sharepoint.com/personal/*microsoft_com/documents"

This case should deal with files in the personal library of a business OneDrive account that are also not located in any subfolder, which is again a bit of a niche use case.

This case will never be triggered for me because my business account email address doesn't end with "microsoft.com", and as far as i can tell, the "microsoft_com" is a part of the account email address. Maybe the line should be

`Case LCase(ThisWorkbook.Path) Like "https://*my.sharepoint.com/personal/*/documents"`

but of course, this could then also trigger for subfolders of the personal library of a business OneDrive account that are called "documents", so this case could lead to wrong return values. Maybe it would be best to ditch this Case entirely, as it is only a fairly niche case anyway and it might as well be handled by the Case Else part.

3) Case Else

For this case, this is again a very creative solution that I have not seen before, kudos!

Again, there are drawbacks, some of which you already pointed out in your README.md, like that the folder must be open in the Windows Explorer.

Another drawback you didn't mention is that the logic can potentially return the wrong path if multiple explorer windows to multiple different folders with the same name are open.
In this case, because you included the If Not fso.FileExists(strPath & "\" & ThisWorkbook.Name) Then Exit Function check, it can likely happen that nothing is returned.

However if multiple of these folders also happened to contain a file that has the same name as the current workbook, the function might return a wrong path. I do not see a way of overcoming this issue, unfortunately.

Issues with the provided implementation of the DecodeURL function:

  1. The function uses early binding and requires a reference to the Microsoft HTML Object Library, maybe this should be mentioned in the documentation. Alternatively it would be even better to use late binding for portability.

  2. Even with the reference added, this function causes an automation error for me at this line:

htmlDoc.parentWindow.execScript "document.getElementById('result').innerText = " & _
                                    "decodeURIComponent('" & URL & "');"

even for some very simple inputs like DecodeURL("%F6"). I don't know why that is, because for example DecodeURL("%20") works without issues but I suggest using a completely different implementation of DecodeURL.

Solution 3 (using SendKeys):

Unfortunately, I could not properly test this solution because it was flagged as dangerous by my antivirus, and calling GetThisWorkbookLocalPath3 led my antivirus to immediately terminate the entire Excel process.

However, I know that using SendKeys is a viable option, and it is actually not necessary to use PowerShell or CreateObject("Wscript.Shell").Run at all. For a detailed description of how this can be achieved in a better way, by using the built-in Application.SendKeys please have a look here.

More General Remarks

In your README.md, you write:

..., it is virtually impossible to convert the URL returned by ThisWorkbook.Path to a local path by string processing.

This is not quite true. You just need a lot more information about the OneDrive setup than what is available in the environment variables or the registry. However, there is enough information to make this work for every single case in the OneDrive settings files. On Windows, they are located in %localappdata%\Microsoft\OneDrive\settings.

The exact string processing process and information extraction from the settings files is unfortunately extremely complicated and goes FAR beyond the scope of this issue.

However, there are already two solutions that use this approach available under the MIT license, and I encourage you to try them out. More information on both of these can be found here: https://stackoverflow.com/a/73577057/12287457

Final Remarks and Questions

Of your solutions, I think the first one is the best, as its limitations and drawbacks are the most manageable.

However, the solutions presented here: https://stackoverflow.com/a/73577057/12287457 have none of these drawbacks and in my opinion even this solution which uses the registry has less drawbacks than any of the solutions presented in this repository.

Considering this and since obviously a lot of thought went into the solutions from this repository, I have some questions:

Did you look for solutions online before coming up with these approaches?

If yes did you find the ones I linked? Specifically these two:

Did you try these solutions and did they work?

If not, can you share the case where they didn't work?

If yes, why did you come up with your alternative solutions?


Finally, sorry for the very long issue. Nonetheless, I hope you find it helpful. If you have any questions about any of the things I mentioned in this issue, please don't hesitate to ask and I will do my best to clarify!

@Excel-VBA-Diary
Copy link
Owner

Thank you for your prompt and thorough report.
There are so many solutions to this problem, but I have been frustrated that none of them work, especially when it comes to OneDrive synced to SharePoint.
As already mentioned, each solution has its prerequisites, and as you point out, not all barriers have been removed.
I have tried the VBA code you posted on GitHub, and since I don't have a MAC environment, my attempts are in a Windows environment, but I can confirm that it works very well.
I hadn't thought of the idea of focusing on OneDrive setting files, for example, a folder like:
C:\Users<USER-NAME>\AppData\Local\Microsoft\OneDrive\settings
Actually, I thought there was information about OneDrive somewhere in Windows and was looking for it, but didn't know it was here.
It is very informative and I would like to review it.
Again, thank you very much for your report.

@Excel-VBA-Diary
Copy link
Owner

The repository has been updated based on your comments. In GetThisWorkbookLocalPath, we have changed to recommend our own GetLocalPath. There are still some issues with this GetLocalPath, but it is mostly practical in our environment.
GetThisWorkbookLocalPath:
https://github.com/Excel-VBA-Diary/GetThisWorkbookLocalPath/blob/main/README(en).md
GetLocalPath:
https://github.com/Excel-VBA-Diary/GetLocalPath/blob/main/README(en).md
Thanks again for your comments, they are very much appreciated.

@guwidoe
Copy link
Author

guwidoe commented Feb 26, 2024

Hi,
I have seen your new repository, good work on implementing that solution too! There are still many issues with your new implementation. For example, on my system, I currently get a "key already exists" error from the dictionary.
Are you interested in developing it into a fully reliable solution?
If so, you will redo a ton of work that has already been done (See the two solutions I linked in my initial comment)
I won't be able to report all the issues that will likely pop up because that in itself would be a lot of work, I can only suggest that you look through the things we uncovered while going down the rabbit hole here:
cristianbuse/VBA-FileTools#1
cristianbuse/VBA-FileTools#2

Unfortunately, even these threads don't cover all the intricacies and the solutions have developed quite a bit since. For example, the .dat file no longer exists in the settings folder of the modern OneDrive app and now the .db (SQLite) file has to be read.

@Excel-VBA-Diary
Copy link
Owner

Sorry for the late reply.
Thank you for your very helpful comments.
I have reviewed my GetLocalPath function code in reference to your comment.
I believe that this code has avoided the "key already exists" error.
However, there is still room for improvement and I will continue to test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants