Opened 11 years ago

Closed 11 years ago

Last modified 13 months ago

#8906 closed patch (fixed)

OS/2 patches for posix-fs

Reported by: SF/lvzuufx Owned by: fingolfin
Priority: normal Component: Port: OS/2
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)

posix-fs_os2.diff (1.9 KB ) - added by SF/lvzuufx 11 years ago.
posix-fs_os2-ver2.diff (1.8 KB ) - added by sev- 11 years ago.
Cleaned up patch
os2fs.diff (1.7 KB ) - added by SF/lvzuufx 11 years ago.
posix fs for OS/2
os2fs_r2.diff (1.3 KB ) - added by SF/lvzuufx 11 years ago.
posix fs for OS/2 r2

Download all attachments as: .zip

Change History (23)

by SF/lvzuufx, 11 years ago

Attachment: posix-fs_os2.diff added

by sev-, 11 years ago

Attachment: posix-fs_os2-ver2.diff added

Cleaned up patch

comment:1 by sev-, 11 years ago

Owner: set to fingolfin

comment:2 by sev-, 11 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 SF/lvzuufx, 11 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 fingolfin, 11 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 fingolfin, 11 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 SF/lvzuufx, 11 years ago

Hi/2.

I would like to test your codes than to provide a patch myself.

Thanks.

comment:7 by fingolfin, 11 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 fingolfin, 11 years ago

Status: newclosed

comment:9 by SF/lvzuufx, 11 years ago

Hi/2.

It works fine. ^^

Thanks a lot.

comment:10 by fingolfin, 11 years ago

Great :) Thanks for your help!

by SF/lvzuufx, 11 years ago

Attachment: os2fs.diff added

posix fs for OS/2

comment:11 by SF/lvzuufx, 11 years ago

Hi/2.

Some recent changes broke os2fs functionality.

So I attach patches again.

File Added: os2fs.diff

comment:12 by SF/lvzuufx, 11 years ago

Status: closednew

comment:13 by fingolfin, 11 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 :).

by SF/lvzuufx, 11 years ago

Attachment: os2fs_r2.diff added

posix fs for OS/2 r2

comment:14 by SF/lvzuufx, 11 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 fingolfin, 11 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 fingolfin, 11 years ago

Resolution: fixed
Status: newclosed

comment:17 by SF/lvzuufx, 11 years ago

Hi/2.

It works fine.

Thanks a lot. ^^

comment:18 by digitall, 13 months ago

Component: Ports

comment:19 by digitall, 13 months ago

Component: PortsPort: OS/2
Note: See TracTickets for help on using tickets.