#8906 closed patch (fixed)
OS/2 patches for posix-fs
| Reported by: | SF/lvzuufx | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | Port: OS/2 |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
Hi/2.
This patch enable a usage of drive letter with posix-fs backend on OS/2.
KO Myung-Hun
Ticket imported from: #2043093. Ticket imported from: patches/1011.
Attachments (4)
Change History (23)
by , 16 years ago
| Attachment: | posix-fs_os2.diff added |
|---|
by , 16 years ago
| Attachment: | posix-fs_os2-ver2.diff added |
|---|
comment:1 by , 16 years ago
| Owner: | set to |
|---|
comment:2 by , 16 years ago
I cleaned up the patch. Removed DOS line endings and fixed code formatting.
Against which scummvm version did you produce this patch? It does not apply cleanly to the trunk.
Max, are we allowing such pollution to our generic backend? Or perhaps it would make sense to produce os2-specific fs?
Myung-Hun, other than that the patch looks OK to me. File Added: posix-fs_os2-ver2.diff
comment:3 by , 16 years ago
Thanks for your review.
This patch is based on 0.11.1 sources.
I'm sorry I don't use svn for scummvm.
It's good my patch looks OK to you.
And maybe, do you want additional work to apply it ?
KO Myung-Hun
comment:4 by , 16 years ago
I'll try to take a close look tonight.
Normally, I would not like to add #ifdef code to our POSIX FS backend, though. Or any FS backend, for that matter. The drawback of that is that currently we have quite some code duplication between FS backends. But maybe we can reduce that by using subclassing. So, we could add a new OS2 FS backend, which subclasses the POSIX backend and only overloads those few bits that have to be changed.
comment:5 by , 16 years ago
Sorry, didn't get around to this earlier :(. Too busy at work to have much time left for ScummVM.
Well, I am not happy about applying this patch as is. However, I think it should be possibly to get an equivalent but cleaner solution into our code, by subclassing the POSIX backend. KO Myung-Hun / lvzuufx: I could adapt your code a bit to do just that, could you do some testing then? Or alternatively, if you want, you could provide a patch yourself (essentially, move the POSIX class def to a new header file backends/fs/posix/posix-fs.h, and then make a new backends/fs/os2/os2-fs.h which subclasses that. And then modify backends/platform/sdl/sdl.cpp to use the new OS2 FS backend on OS2.
comment:6 by , 16 years ago
Hi/2.
I would like to test your codes than to provide a patch myself.
Thanks.
comment:7 by , 16 years ago
OK, after reviewing the whole thing, it wouldn't be quite as simple to use a subclass of POSIXFSNode as I thought -- mainly because the POSIXFSNode code has to instantiate objects in lots of spots, so with the current way the code works, I'd have to duplicate the whole POSIXFSNode code, rendering the idea of subclassing mostly pointless.
There *are* ways around that, but it requires some refactoring to how POSIXFSNode (or even FSNodes in general) work. And I don't want to spend that effort right now. So instead of letting this patch linger, I commited a modified form of it. KO Myung-Hun - lvzuufx - if you could try to see whether it works with the latest trunk version, that would be helpful :)
comment:8 by , 16 years ago
| Status: | new → closed |
|---|
comment:11 by , 16 years ago
Hi/2.
Some recent changes broke os2fs functionality.
So I attach patches again.
File Added: os2fs.diff
comment:12 by , 16 years ago
| Status: | closed → new |
|---|
comment:13 by , 16 years ago
Thanks, but I have some questions. I understand the part which fixes the compiler problems, oops. But I am confused about some of the others. Why do you have to modify common/str.cpp ?
Also: You force a trailing slash for the "volume paths". That is, with your code, the root contains for each drive an entry with path like "A:/" and name "[A:]". Now: Does path = "A:" (without the slash) not work? I would prefer to use that, if it works...
Another oddity: If one creates the root node, then the "volume nodes" have display names like "[A:]" and path "A:/". But if one uses getParent() on e.g. "A:/FILE.TXT", one will get displayname "A:" (instead of "[A:]").
So, I also have to ask: Is there a particular reason to use "[A:]" instead of "A:"?
I commited some changes, different from yours, which should fix the compile problems; please test, and tell me if those changes are enough; if not, I am happy to get a new patch from you, but I'd prefer to not alter common/str.cpp, if can avoid it :).
comment:14 by , 16 years ago
Hi/2.
In case of common/str.cpp, normalizePath() remove a slash of 'X:/'. So I should modify it. But this time, I added a condition for OS/2 before calling normalizePath().
On OS/2, "A:" and "A:/" is different. "A:" means a current dir of A drive, but "A:/" means the root of A drive.
And conventionally, A: style is used to change a drive. And [] is used to emphasize it's drive.
If 'A:' is used instead of 'A:/', some problems can occur.
1. We cannot arrive at the root of A drive through 'Go Up'. Because the current logic recognize it as relative path.
2. As said above, 'A:' means a current dir of A drive. BTW, '/' is added to _path, it means a root of A drive. So it can point a wrong place out.
In case of displayname, it can get 'A:/' instead of 'A:' which you said.
Anyway it is different from [A:]. BTW displayname returned by getParent() is used at somewhere other than browsing ?
I attach another patch without modifying common/str.cpp. ^^
Finally, if you want consistency of path, I prefer 'A:/' to 'A:'. And if you think difference of displayname is a important problem, you can use 'A:/' instead of '[A:]'.
Ah, I modifed 'char *drive_root' to 'char drive_root[]' because it generated warning.
File Added: os2fs_r2.diff
comment:15 by , 16 years ago
Thanks once more for your contribution. I commited the fix to SVN. If there are further problems, please create a new tracker item.
comment:16 by , 16 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
comment:18 by , 6 years ago
| Component: | → Ports |
|---|
comment:19 by , 6 years ago
| Component: | Ports → Port: OS/2 |
|---|
Cleaned up patch