-
Notifications
You must be signed in to change notification settings - Fork 710
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
UNC path name handling #345
base: master
Are you sure you want to change the base?
Conversation
cbae804
to
1bbb914
Compare
GeneralHow to useOne can
Works stable DownloadUNC support is available from my private branch and |
1bbb914
to
32af2ab
Compare
Rebased onto 9525928 |
60b9c23
to
d0e6a1b
Compare
After a 5 month test, daily use and ironing out a few glitches lets make it ready for review. This PR is a big one I know ;-) and it it really does changes with the innner hydraulics.
this really works well. Lets see if someone is brave enough to tackle with this one |
Happy to look, although right now I'm not sure what I'm looking at...is there a Cliff's Notes summary of what's happening? I could use Goto to launch a window in a UNC path. That doesn't seem to populate the drive list with a virtual drive though - how does the virtual drive logic work? |
Added two convenience commits, because during development I didn't care
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still going through these, but it seems useful to share the comments I have now.
@@ -104,7 +104,9 @@ INT atoiW(LPWSTR sz); | |||
#define MAXMESSAGELEN (MAXPATHLEN * 2 + MAXSUGGESTLEN) | |||
|
|||
#define MAX_WINDOWS 27 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value seems very suspicious but it looks like the current code can handle multiple windows to the same drive. Do you know of any reason for this value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't stumble across this one, but sure it points to the max number of open windows.
I assume with 10 numbered and 26 letter drives, we currently will not be able to have all open at the same time.
But they must have had similar problems, when one opened a second window to one drive, then you could also not have your favorite 'Z' drive shown.
But 27 windows open at the same time is too much anyhow. One can not handle this, so it is a limit i would live with.
// Search the list of slots if path would be parent of already existing drive == circularity | ||
BOOL FindUNCLoop(LPCTSTR path) | ||
{ | ||
for (DWORD dwDriveIndex = OFFSET_UNC; dwDriveIndex < MAX_DRIVES; ++dwDriveIndex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please declare the variable at the start, C89 style.
(I know this sounds opinionated but I promise this is pure pragmatism - it lets the code compile with older compilers, allowing code to move back to the retro branch, etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry no. We are in 2023 and applying a 34 year old coding style seems odd to me.
It makes things unreadable. One day we have to face the present ;-)
I know you need the old compiler for you W2K & WXP branch. Anyhow the whole Winfile has lots of C89 violations and also contains wfgoto.cpp which you have to exclude from the makefile to get it compiling.
So you have to anyhow change something, when you bring it to your branch. To ease your life you could start with https://github.com/schinagl/winfile/tree/malxau_hardlink_junctions which I took from you and made the whole hardlink/sysmlink stuff compile with C89.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stepping back a bit, my motivation is to maximize the potential contributors and potential users of the project. Reasonable people can disagree how to best achieve those goals, but those goals underpin everything I'm doing in this project.
When I made the previous comment, I believed that #165 had removed C99 variable declarations and replaced them with C89, and C99 was being gradually reintroduced. After reviewing this and related PRs, it appears they never merged, which takes care of the core of my objection. Note these did not remove wfgoto; they attempted to use earlier C++ standards to support compiling on XP.
Reading the comments in the PRs though stings a bit, because it shows potential contributors being discouraged and openly giving up on the project. It's not surprising that a project like this would attract retro enthusiasts, and keeping a large group of contributors implies having people with different goals working together. I hope that you and I share that perspective: if we didn't, it would be easy enough to archive this repo, leaving it read only, and let anyone maintain their own fork with no collaboration. Working together means accepting that not everyone wants the same thing, but if we have enough overlap, we can still improve things for each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you mean that people do not develop on this repo because it is not in C89 ??? This is hilarious.
I am totally confused reading your above writing.
fyi: @craigwi
Any news here? |
Have you tried this branch? @craigwi: What do you think? |
cf016eb
to
d332a7a
Compare
Any news here? Review results? |
The original PR itself has no unnecessary reformatting, but yes I had to merge @malxau reformatting and beautifying into it because otherwise there would be conflicts and it would not merge for github. Sure I can put my changes on top of the master once @malxau is done with his beautifying PRs, so that you see the bare changes. (I am back on my computer by begin of September)
|
6b92b0b
to
bf383b7
Compare
…apable drives, because they are UNC drives
Any news here? One can download binaries from here |
Any news here? One can download binaries from here |
Hi @schinagl, I ran into a problem with a basic test. File.Goto Directory to a network share worked. Then running Disk.Select Drive shows two things, my C drive and the network share. That's fine. The problem is that the items are reversed. When I select the c: drive, I get the network share and vice versa. The order in the dropdown list on the toolbar seems ok. Next I when on my c: drive and use File.Goto Directory to the same network share, nothing happens; that is, the window doesn't change to the network share. |
Thx for looking into it
This is odd as it works on my side, but I have many drives, network drives and 3 UNC drives
Can confirm this. This wasn't a use-case on my side, but we can fix this |
Fixed 2) from above: Related: A limitation ever since was and will be: One can not UNC-map to a path, when it would be the parent of an already existing UNC mapping, e.g. \\foo\bar for existing UNC mapping \\foo\bar\share |
0886188
to
3120383
Compare
Take care of UNC drives beeing not the last window(s)
…ives after start of Winfile (F5 would heal this)
I have played a around quite a lot, use Winfile as all/every day file-manager
Unfortunatley I was not able to reproduce
How did you get there? I tried it on a clean machine, with no drives, but only C: And whatever I tried I was not able to run into the problem. Are you sure you took the most recent version? I had a similar glitch in February 4, when sorting was enabled in the .dlg files, but it must not. But this is long gone. Did you recompile from this very branch? I am really out of ideas, since, as usual with those things, it works flawlessly on my side. |
I am leaving this Winfile Repo. See #411 |
…lus one. E-.g. Going back from c:\ba via .. didn't result in showing c:\ on the left pane This only affects the UNC capable version (cherry picked from commit d81ee37)
# Conflicts: # src/lang/res_ja-JP.rc # src/lang/winfile_de-DE.dlg # src/lang/winfile_he-IL.dlg # src/lang/winfile_ja-JP.dlg # src/lang/winfile_pl-PL.dlg # src/lang/winfile_tr-TR.dlg # src/lang/winfile_zh-CN.dlg # src/wfutil.c
# Conflicts: # src/wfinit.c
# Conflicts: # README.md
# Conflicts: # src/wfutil.c
# Conflicts: # src/wfcopy.c
ff5dd36
to
908c426
Compare
# Conflicts: # src/wfgoto.cpp # src/wfinit.c
i have had success with Disk > Connect to a network drive . is that the same thing as all this? |
No. You connected a drive. Please go to my fork https://github.com/schinagl/winfile/releases Take the UNC capable winfile from there: press CTRL-G and enter a UNC path where you are already authenticated. |
285ddd4
to
f6aef4a
Compare
Basic UNC path support