#9462 closed patch
OSX: Fix & localize native browser dialog
| Reported by: | fingolfin | Owned by: | criezy |
|---|---|---|---|
| Priority: | normal | Component: | Port: Mac OS X |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
The attached patch fixes a problem with the native browser dialog on OS X (namely it ignored the dirBrowser flag), and also enables localization support in it. Indeed, previously the browser title string was completely ignored, now it is used. And the default button of the browser dialog now is labelled "Choose".
It also removes the use of several deprecated AppKit methods.
Ticket imported from: #3582260. Ticket imported from: patches/1567.
Attachments (3)
Change History (20)
by , 11 years ago
| Attachment: | osx-browser.patch added |
|---|
comment:1 by , 11 years ago
comment:2 by , 11 years ago
tdhs: Regarding 1 and 3: I followed the instructions from http://wiki.scummvm.org/index.php/Developer_Central which explicitly state "If you do not have write access, you should use our patch tracker to submit your contributions." and do not mention github. And I do not like submitting patches in private -- patch trackers, mailing lists, pull requests are public, that's much better... also, I didn't know who is responsible (if at all) for the OS X port these days, so it seemed prudent to follow the specific wishes of the ScummVM team.
And yeah, it is kinda easier for me to just submit this as a patch, as opposed to creating a fulll fork of ScummVM just for a few lines of changed code. I only made this patch as a favor for a friend anyway :).
Regarding 2: I only tested this on a 10.7 system; Apple sadly makes it kinda hard to test with older SDKs (latest XCode here only has 10.7 and 10.8 SDKs, and they even are trying to make me use the 10.8 SDK by default -- which is really idotic, given that I work on a 10.7 machine). But according to the OS X documentation (and unless I missed a call), all the calls I added have been around since 10.0, so they should be fully backward compatible -- but I cannot test this, as I don't have access to such ancient systems.
[Side note: The oldest Mac OS X version I have seen in the last 4 years was 10.4, too, and IMHO it makes no sense to try to support 10.3 and older if there are no explicitly known testers (or even users) for these old versions... but of course that's just my personal point of view, and of course it's up to you to decide what you try to support and not! Just sayin', as a Mac OS X user and developer]
comment:3 by , 11 years ago
I looked quickly at the patch and it looks good. I always found confusing to have a dialog where we can select files when adding a game, so this definitely improves the situation. Having the title also helps.
I have a few questions/remarks though:
Why leave the '[encStr release];' line commented out in the BrowserDialog constructor? Since you use a convenience constructor for that NSString it should be autoreleased, so that release is not needed. Leaving that line may confuse somebody looking at the code, wondering if there is a memory leak or not.
I believe NSString stringWithCString:encoding: was introduced in MacOS X 10.4. So this may be an issue if we want to keep support for older systems. However a quick grep shows we use it in the Sparkle code as well, and we use initWithCString:encoding: for the menu code. So that might be fine.
comment:4 by , 11 years ago
Oh, and another comment: since you use the _() macro to translate "Choose", in theory that file should be added to po/POTFILES as well. That will work without that change because currently "Choose" is already present in the scummvm.pot file from other sources.
comment:5 by , 11 years ago
fingolfin: 1: No problem as it is a single commit anyway. The Development Central notes have not been fully updated since the SVN to Git migration and workflow changes. I'll update them accordingly if I have permissions or send a note to Core Team to edit.
AFAIK, Oystein is still the main OSX maintainer and he did build the v1.5.0 release packages as the scummvm-devel mailing list archive records. I suspect you mean that his name is missing from the "assigned" list curently. That is associated with the problems with the SF.net bug and other trackers atm associated with the migration problems to the new Allura platform, and should get fixed at some point soon (My permissions also got fouled up).
comment:6 by , 11 years ago
2. Since I'm not an OSX user, I don't really know either.. And our wiki page for this is light on information about supported versions: http://wiki.scummvm.org/index.php/Mac_OS_X
3. I was not suggesting sending a patch in private. Just that as I doubt vinterstum is monitoring the patch tracker, a quick "headsup" e-mail to prompt him might be useful. Ditto for Github Pull Request... *sigh* Wish sf.net and Github had better and easier notification by keyword or similar, and people set these up! :)
Thanks again for this. Hopefully he will look at this.
comment:8 by , 11 years ago
I will add one note on the compatibility with older systems: the Apple documentation confirms NSString stringWithCString:encoding: and initWithCString:encoding: were both added in OS X 10.4 (see https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Foundation/Classes/NSString_Class/Reference/NSString.html).
As noted, the first one is already used in the Sparkle code. I wrote the second one is used in the menu code but this is actually a change made by this patch. The current code in master does not use it.
I still have an old G3 mac with OS X 10.3 and last time I tried the ScummVM code compiled on it (with Sparkle disabled). I have not tried compiling with this patch. However I don't use ScummVM on that computer and it is only useful to me for checking the behaviour of old mac games with their original interpreter. There are probably not many people still using ScummVM on such an old system. I am not opposed to dropping compatibility with older system but in the end this is vinterstum call. And if we wanted to keep the compatibility I believe we could easily modify the patch to keep the compatibility but still get the improved native browser behaviour.
comment:9 by , 11 years ago
As a PowerPC user, I'm stuck on 10.5.8, and I hope that support doesn't go away anytime soon.
I just did a quick build and test of this patch against the current master branch head (2c5769c23) and there were no problems, as expected.
Tested with a G4 running 10.5.8 using English.
comment:10 by , 11 years ago
re commented out [encStr release] line: That is a mistake, not intentional, that line should indeed go.
re [NSString stringWithCString:encoding:] and its init* sibling -- yeah, that method is 10.4 upward, I overlooked it, sorry. But as people here already discussed, it is already being used by other code. Anyway, an easy way to resolve that is to simply use CFStringCreateWithCString, which has been available since 10.0 (and internally CFString and NSString objects are exactly the same).
Re backward compatibility: Since 10.4 was the last OS to work on PPC machines, compatibility with that (and 10.5) would indeed be nice, but anything older still seems pointless to me. But again, that's just me.
re po/POTFILES: This single string is already used in the non-native version of the browser dialog code, so it doesn't seem necessary to add it. Alas, it is trivial to do so, and I am sure that if you consider it important, you can easily do this at the same time or right after applying this patch, should you choose to apply it at all.
Attached a modified patch which hopefully addresses the concerns (except for the POTFILES one).
by , 11 years ago
| Attachment: | macosx-updates.patch added |
|---|
Patch sparkle code to use CFStringCreateWithCString, too
comment:11 by , 11 years ago
While I was at it, I also added a small patch which changes the Sparkle code to use CFStringCreateWithCString instead of NSString stringWithCString:encoding:
comment:12 by , 11 years ago
deledrius: That would be up to the platform porter, but we do generally try to support older OS versions unless there are very good reasons to drop support. I would hope vinterstum responds this weekend to the message I sent to the list...
comment:13 by , 11 years ago
| Owner: | set to |
|---|
comment:14 by , 11 years ago
I have assigned this to criezy as he has a 10.3 machine and has volunteered on the list! :)
Seriously, criezy if you can test and commit this weekend, that would be optimal. Thanks!
comment:15 by , 11 years ago
Thank you. I have now committed and pushed both your browser dialog and sparkle patches. There was only an include of NSURL.h missing for compilation on older systems, which I have now added.
The code compiles on both my OS X 10.3 and 10.8 machines and works as expected on both.
comment:16 by , 11 years ago
| Status: | new → closed |
|---|
comment:17 by , 5 years ago
| Component: | → Port: Mac OS X |
|---|
fingolfin: Thanks for this patch. A couple of questions: 1. Any specific reason that you didn't do it as a GH Pull Request? I am assuming that it was due to being a single commit, so a bit overkill and this was quicker for you. 2. Have you tested/Do you know if this will work with the older toolchains and older OSX versions? I think we support back to 10.2/3 IIRC. 3. Have you already notified/sent this to vinterstum by e-mail? clone2727 has said it looks fine, but will need him to review for the older release/toolchain issues.
Thanks again for this.