305061 - OS/2: use RWS to improve platform integration

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Rich Walsh]

This is Phase I of a project to improve OS/2 platform integration using my
Remote Workplace Server (RWS) libraries.  Phase I uses RWS to fetch icons and
helper-apps from the WPS.  Later phases will use RWS to implement symbolic
links, a helper-app selection dialog, and more.

To implement these patches, you will need the RWS v0.70 development package
which can be obtained from:  http://e-vertise.com/rws/rws070.zip

Note:  while the build process is dependent on files from this package, the
final product is not.  If the RWS libraries are absent, the program will be
fully functional but will lack the additional features RWS provides.

See subsequent comments for build instructions and known issues.

Comment 1

[Rich Walsh]

Attachment: mozilla/xpcom/rwsos2/Makefile.in

Comment 2

[Rich Walsh]

Attachment: mozilla/xpcom/rwsos2/nsIRwsService.idl

Comment 3

[Rich Walsh]

Attachment: mozilla/xpcom/rwsos2/nsRwsService.cpp

Comment 4

[Rich Walsh]

Attachment: mozilla/xpcom/rwsos2/nsRwsService.h

Comment 5

[Rich Walsh]

Attachment: patches to allmakefiles.sh & xpcom/Makefile.in

Comment 6

[Rich Walsh]

Attachment: patches to uriloader/exthandler/os2/ & modules/libpr0n/decoders/icon/os2/

Comment 7

[Rich Walsh]

Attachment: patch to xpfe/components/directory/

This still needs preprocessing directives to make its changes OS/2-only.  It is
included for the benefit of those who create private OS/2 builds.

Comment 8

[Peter Weilbacher]

Very interesting, thanks for posting this, Rich. I only took a glance for now
and didn't have time to even try a build yet, but how does it find rws.h? I.e.
where do you recommend to unpack the RWS distribution for building Mozilla?
Also, did you consider a --disable-rwsos2 switch for configure so that people
without it can still build?

Comment 9

[Rich Walsh]

Build info:
- obtain RWS v0.70 from http://e-vertise.com/rws/rws070.zip
- add its top-level directory (RWS) to your CPLUS_INCLUDE_PATH in setmozenv.cmd
- in the same directory, run rwsutil07.cmd to register the RWS07 WPS class
(note:  all RWS-enabled mozapps will use the RWS dlls contained in this
directory;  if you have earlier versions of RWS v0.70, please delete them)
- create a directory named "mozilla/xpcom/rwsos2"
- copy the 4 rwsos2 source files to this directory
- apply the patches

Notes:
- The rwsos2 directory probably doesn't belong in xpcom but was put there
because a future version of xpcom/io/nsLocalFileOS2 will require
nsIRwsService.h.  The current arrangement ensures that this header is built from
the IDL before nsLocalFileOS2 is compiled.  How this can be accomplished when
rwsos2 is elsewhere remains to be seen.

- This version of "Phase I" includes two features not seen previously:
 * nsRwsService now caches the icons & helper-app info associated with file
extensions;  in most cases, this provides a dramatic reduction in overhead.
 * when RWS is present, nsMIMEInfoOS2 now uses the WPS to start all helper-apps,
including those specified in a mailcap file.  The effect is identical to
dropping a file on a program file/object's icon.  Any commandline parameters or
other settings entered in the file or object's WPS Properties notebook will be
honored.

- RWS is licensed under the Mozilla Public License and is freely
redistributable.  Anyone who distributes an RWS-enabled app should include the
RWS dlls & rwsutil07.cmd in the same directory as the main executable.

Comment 10

[Rich Walsh]

(In reply to comment #8)
> how does it find rws.h? I.e. where do you recommend to unpack the RWS
> distribution for building Mozilla?

You posted your comment 5 minutes before I finished my discourse (Comment #9). 
The RWS package can go almost anywhere - but definitely not in the mozilla tree.
 See the "Build info" section of comment #9 for details.

> did you consider a --disable-rwsos2 switch for configure so that people
> without it can still build?

I don't expect that this will be approved & checked in for a _long_ time (if
ever).  Meanwhile, only those who have chosen to apply these patches will need
the headers.  We can discuss this further as things progress.

Comment 11

[timeless]

we do have a mozilla/other-licenses/

but, can the mozilla/xpcom/rwsos2 stuff live in mozilla/widget/os2/ ? it kinda 
feels more widgety to me.

Comment 12

[Andy Willis (abwillis)]

other-licenses doesn't seem right from the perspective that no one else really
would be able to make use of it... if the idl/header issue that currently
requires the xpcom placement can be overcome then I would suggest the widget/os2
location. I don't know the actual definition of widget or whether this falls
under it but if it were in widget/os2/rws then it would be contained in such a
way as to know it was specifically an os2 addition.

Comment 13

[Rich Walsh]

(In reply to comment #11)
> we do have a mozilla/other-licenses/

All code intended for the mozilla build-tree has the standard tri-license.  RWS,
which is not a mozilla.org project, currently bears the MPL only.  I'll change
it to the tri-license if that will facilitate the use & distribution of its
headers & binaries.

> but, can the mozilla/xpcom/rwsos2 stuff live in mozilla/widget/os2/ ?
> it kinda feels more widgety to me.

rwsos2 was put in xpcom as a temporary workaround for a dependency problem.
In the next phase of this project, nsLocalFileOS2.cpp will require
nsIRwsService.h.  In turn, nsRwsService.cpp depends upon various XPCOM headers.
 These dependencies require files to be processed in this order:
   nsIRwsService.idl--> XPCOM--> nsRwsService.cpp

Identify a different way to do this and rwsos2 can go anywhere (personally, I
thought something like "modules/libos2" was the most appropriate).

Comment 14

[Rich Walsh]

Attachment: bugfix for nsRwsService.cpp

The original code always returned a mini-icon when fetching the icon associated with a file extension;  this revision gets the large version when requested.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

isn't modules/ deprecated?

Comment 16

[Peter Weilbacher]

I have looked through all of the code in the meantime and cannot find anything wrong with the buildconfig and the largest part of the C++ changes. Other platforms than OS/2 are completely unaffected. I don't understand why it would be so important to have it in widget/ or somewhere else than in xpcom/, after all there also is xpcom/MoreFiles/ for the Mac stuff...

It is also well tested by now, as I have put this into my PmW-Fx and PmW-Tb packages (unofficial FF and TB for OS/2) as well as my unofficial SeaMonkey 1.0beta and users are really happy with this feature.

Three issues:
- The patches write several strings that I think are visible to users.
  Examples are 
  + nsRwsService::HandlerFromPath() (attachment 204105 [details])
     _retval.AssignLiteral( "MM Image Viewer");
     (and several similar AssignLiterals for different WPS classes)
     _retval.Assign( "WPS default");
     _retval.Append( NS_LITERAL_CSTRING( " Class Viewer"));
  + WpsGetDefaultHandler() (in attachment 193037 [details] [diff] [review])
    aDescription.AssignLiteral("WPS default");
  All these need to be localized.
- The patch to directory.js (attachment 193038 [details] [diff] [review]) first assumes that RWS
  is available, which will not be the case on the main platforms and
  hence should be reversed. Would preprocessing and #ifdef XP_OS2 be
  the way to go? Let me give it a try to improve this.
- rwsos2.dll needs to be added to the packager list, I think like this

--- mozilla/xpinstall/packager/packages-os2     6 Dec 2005 01:29:42 -0000       1.86
+++ mozilla/xpinstall/packager/packages-os2     29 Dec 2005 15:04:57 -0000
@@ -60,6 +60,7 @@
 bin/components/xpcomctc.dll
 bin/components/jar50.dll
 bin/components/xpinstal.dll
+bin/components/rwsos2.dll

 [browser]
 ; files listed in xpcom (missing in this section) will be installed as part of the browser

Comment 17

[Rich Walsh]

(In reply to comment #16)
> I don't understand why it would be so important to have it in widget/ or
> somewhere else than in xpcom/

I put rwsos2 in xpcom because I expected to use it extensively in nsLocalFileOS2 (and xpcom shouldn't be dependent on other components).  However, since I have ceased new development this is no longer an issue.  It can be moved to widget if that will make it more acceptable to whomever.

> Three issues:
> - The patches write several strings that I think are visible to users.
>   [snip] All these need to be localized.

I was aware of this need while writing it but didn't know how to use string bundles in C++ code.

> - The patch to directory.js (attachment 193038 [details] [diff] [review] [edit]) first assumes that
>   RWS is available [...] Would preprocessing and #ifdef XP_OS2 be the
>   way to go? Let me give it a try to improve this.
>
> - rwsos2.dll needs to be added to the packager list [...]

If you know how to fix these two, go for it.

FWIW...  while I'm rather fed up with the Mozilla project, I remain committed to fixing bugs & shortcomings in code that I've already submitted.  I'll look into moving rwsos2 into widget and - if someone can point me in the right direction - making various strings localizable.

Comment 18

[Mike Kaply [:mkaply]]

(In reply to comment #17)

> FWIW...  while I'm rather fed up with the Mozilla project, I remain committed
> to fixing bugs & shortcomings in code that I've already submitted.  I'll look
> into moving rwsos2 into widget and - if someone can point me in the right
> direction - making various strings localizable.
> 
Rich,

I'm really sorry that you had the experience you had with the file/meta data bug. Sometimes too many cooks really do spoil the broth in this project as happened with that bug. I have brought up that bug to other people as an example of how to turn off a contributor to this project.

You're knowledge of some of this code and OS/2 internals has been invaluable and I would be very sad to see you stop participating. You are a valued member of this community from an OS/2 perspective.

If I have done anything personally to turn you off to this project, please let me know so I can remedy it.

Here's an example of CPP string bundles:

http://lxr.mozilla.org/seamonkey/source/extensions/gnomevfs/nsGnomeVFSProtocolHandler.cpp#226

Comment 19

[Peter Weilbacher]

The issues in comment 16 were more thought to be notes to myself. (And one message should have been that I think that it should _not_ be moved to widget.) But if you have some time and are in the mood to do the string bundle stuff that would be even better. :-)

Comment 20

[Rich Walsh]

Mike, thanks for the kind words -and the stringbundle example.
Peter, I'm working on replacing the literals with a stringbundle & should have something fairly soon.

As to where RWS should go... I now have an alternate version that works in widget/os2.  I'll leave it to someone else to decide which should be used.

Comment 21

[Andy Willis (abwillis)]

Along the lines of "shortcomings" is there a way to get ftp sites to act the same way that local files do when xultree is selected?  It would certainly be nice.

Comment 22

[Peter Weilbacher]

In what way? WPS-like menus are out of the question for remote objects (AFAIU) and the icons are the part of this enhancement that makes the attachment process act weird.

Comment 23

[Andy Willis (abwillis)]

I am thinking of icons to a lesser extent but more importantly to me would be the ability to right click on a file and have it do something (even if just the option to 'save target as').  As it stands having xultree selected prevents being able to download files that the browser wants to display (.cmd files, .txt files, .htm files, etc).  My hope was that rws would recognize these and give me options (sort of like IE allows copy and paste from ftp sites but more so as I could select any action I would get from the wps when right clicking on an object).

Comment 24

[Mike Kaply [:mkaply]]

Rich,

If you're still around, can you explain what you had in mind with xpcom/nsLocalFileOS2?

I'm trying to figure out how we would properly integrate this into the tree.

Thanks!

Comment 25

[Andy Willis (abwillis)]

Attachment: nsRwsService.cpp #include "nsIClassInfoImpl.h"

Bug 330420 forces #include "nsIClassInfoImpl.h" to have to be included in nsRwsService.cpp.

Comment 26

[Peter Weilbacher]

Mike, I don't see nsLocalFileOS2 anywhere in Rich's patch. Or are you asking why it's not getting changed by this patch?

Comment 27

[Rich Walsh]

(In reply to comment #24)
> If you're still around

I still get mail :-)

> can you explain what you had in mind with xpcom/nsLocalFileOS2?

Originally, I was planning on integrating WPS Abstract objects such as shadows & program objects into the filesystem.  To support that, it was necessary to put RWS into xpcom since core functionality couldn't be dependent on some optional component.

However, the landscape has changed...  In its current state of development, RWS can go anywhere (or nowhere, for that matter).  Months ago, I reworked it all so it would fit nicely in wdgtos2 - then I got started on another project & forgot to submit the changes.  I'll try to get them together & submit them soon.

BTW... even if the actual WPS interface is never used, this stuff still contains some worthwhile improvements.  Most notably, it lets users select a helper-app for downloads rather than always being forced to save-to-disk.

Comment 28

[Peter Weilbacher]

Comment on attachment 193037 [details] [diff] [review]
patches to uriloader/exthandler/os2/ & modules/libpr0n/decoders/icon/os2/ 

About the changes in nsMIMEInfoOS2::LaunchWithFile:

>+  // if RWS is enabled, have the WPS open the file using the selected app;
>+  // this lets the user specify commandline args in the exe's WPS notebook
>+  if (sUseRws) {
>+    nsCOMPtr<nsIRwsService> rwsSvc(do_GetService("@mozilla.org/rwsos2;1"));
>+    if (!rwsSvc)
>+      sUseRws = PR_FALSE;
>+    else
>+      rv = rwsSvc->OpenWithAppPath(filePath.get(), appPath.get());
>+  }
>+    
>+  // if RWS isn't present or fails, use Moz facilities to run the program
>+  if (NS_FAILED(rv)) {

Should this not be
    if (!sUseRws || NS_FAILED(rv))
instead?

>+    nsCOMPtr<nsIProcess> process = do_CreateInstance(NS_PROCESS_CONTRACTID);
>+    if (process) {
>+      rv = process->Init(application);
>+      if (NS_SUCCEEDED(rv)) {
>+        filePath.Insert('\"', 0);
>+        filePath.Append('\"');
>+
>+        const char * strPath = filePath.get();
>+        PRUint32     pid;
>+        rv = process->Run(PR_FALSE, &strPath, 1, &pid);
>       }

Alex Taylor just reported in the newsgroup that filenames with [] in them trigger the "Specify parameters" box when choosing "Open it with" in the download dialog. To fix, in the part of the patch quoted below, the lines
     filePath.Insert('\"', 0);
     filePath.Append('\"');
could be moved to the top, so that the filePath is always quoted. In my tests that seems to fix the problem and does not have any bad consequences (or did I overlook something).

Comment 29

[Peter Weilbacher]

(In reply to comment #28)
in the part of the patch quoted below

s/below/above/  ;-)

Comment 30

[Peter Weilbacher]

Rich, do you still have the patch that you talked about in comment 27? I think if we ever want to get this into the tree then we should try to move ahead now, even when the trunk isn't really usable.

Apart from the icon/reflow problem that I hope we can solve somehow, I don't see why this feature shouldn't go in. Users and eComStation guys want it...

Even if you cannot make the patch compatible to the current trunk you are welcome to send me the patch from 2006 to me privately and then I try to update it. But I don't have time to start over and try doing what you obviously had done already a year ago...

Comment 32

[Rich Walsh]

(In reply to comment #30)
> Rich, do you still have the patch that you talked about in comment 27? [...]
>you are welcome to send me the patch from 2006 to me privately and then I try
> to update it.

Peter,
I've reviewed my changes & they're really quite minima:  move some files, change a few makefile.in's, move RwsService's class declaration into nsWidgetFactory.  One complicating factor is that I had implemented stringbundle support to replace the static strings in nsOSHelperAppService.cpp (at your request, see comment #16).  I'll try to send you a package that only covers the move to ./widget.  If that builds OK, I can then send you diffs that cover localization.

Comment 33

[Andy Willis (abwillis)]

Attachment: uriloader\exthandler\os2

Recent changes from bug 384374 caused a break when building with these patches.  This patch allows building but it is not handling the preferred application correctly.  WPS default is found correctly but the Mozilla helper field is blank.

Comment 34

[Peter Weilbacher]

I am working with Rich to move the RWS additions from xpcom/rwsos2 to widget/src/os2 and I have a working patch to do that. I am now working to integrate Rich's work on the stringbundle support (to follow l10n rules) and get it functional again after the break, so that we can hopefully get this functionality in before the next FF release.

Will post the full updated patch once done (currently it's not very useful).

Comment 35

[Peter Weilbacher]

Attachment: new full RWS patch

OK, this is the newest version. Changes from previous ones:

- fixes build breaks after bug 384374
- moves the nsRwsService class from xpcom/ to widget/
- adds l10n support (stringbundles, in unknownContentType.properties
  because that is where all the other strings of the download dialog
  are located)
- integrates all (I hope) fixes for problems that were reported
  for my PmW apps
- icon support is added to the code, but switched off because of
  known issues with duplicate attachment icons that lead to crashes
- the patch to xpfe/components/directory/ is no longer there

Comment 36

[Mike Kaply [:mkaply]]

Are there any Os/2 specific properties files we could use for this?

Comment 37

[Peter Weilbacher]

The only one I see is os2charset.properties...

We could invent a new one but is there a better place for it than toolkit/locales/en-US/chrome/mozapps/downloads.

Comment 38

[Andy Willis (abwillis)]

I am getting:
E:/cvs/work/mozilla/uriloader/exthandler/os2/nsOSHelperAppService.cpp:70:27: nsIRwsService.h: No such file or directory
when should nsIRwsService.h be being created?

Comment 39

[Andy Willis (abwillis)]

With the new patch I am getting:
E:/cvs/work/mozilla/uriloader/exthandler/os2/nsOSHelperAppService.cpp:70:27: nsIRwsService.h: No such file or directory
when should nsIRwsService.h be being created?

Comment 40

[Peter Weilbacher]

That should get created from nsIRwsService.idl when calling make in widget/public.

Comment 41

[Andy Willis (abwillis)]

Found nsIRwsService.h in dist\include\widget.
I change the include from "nsIRwsService.h" to "widget/nsIRwsService.h" and it still failed but copying the file to the include directory got it to work.  

Comment 42

[Peter Weilbacher]

Attachment: patch to uriloader/exthandler/Makefile.in

Ah, now I understand what you are trying to tell me. The patch is missing this change to uriloader/exthandler/Makefile.in.

Comment 43

[Christian :Biesinger (don't email me, ping me on IRC)]

+// this is an override of nsExternalHelperAppService's method;
+// after the parent's method has looked for a handler, add
+// an entry for the WPS's handler if there's room and one exists;

why didn't you change GetMIMEInfoFromOS instead?

Comment 44

[Rich Walsh]

(In reply to comment #43)
> +// this is an override of nsExternalHelperAppService's method;
> 
> why didn't you change GetMIMEInfoFromOS instead?

Different timeframes:  this creates, that queries.

This code resolves an OS/2-specific issue.  The "open with" option in Firefox's download dialog is always grayed-out because downloads are incorrectly identified as "application/octet-stream".  The override ensures that all non-executable files are given a usable mimetype & default "open with" handler.

Comment 45

[Christian :Biesinger (don't email me, ping me on IRC)]

I don't understand why you can't do that in GetMIMEInfoFromOS. You said that creates while this queries, but why can't GetMIMEInfoFromOS query as well?

Comment 46

[Rich Walsh]

(In reply to comment #45)
> I don't understand why you can't do that in GetMIMEInfoFromOS. You said that
> creates while this queries, but why can't GetMIMEInfoFromOS query as well?

It's been a couple of years since I got fed up with the self-appointed apparatchiks at mozilla.org, so I'm not as familiar with the patch's context as I once was.

Implementing this in GetMIMEInfoFromOS would change existing behavior.  Putting it here leaves existing behavior unchanged when it produces satisfactory results. 
 
BTW... Mac appears to take this same approach.

Comment 47

[Peter Weilbacher]

Too bad that we didn't manage to get this in a month ago, or so. Now the patch is broken again. Perhaps we should wait until the current work on this part of the code has quieted down and then adapt and get this in some time after the next milestone. As I understand the l10n freeze is not before mid-end of September.

Comment 48

[Peter Weilbacher]

Attachment: RWS integration, new full patch

Finally have found some time to work myself into the confusing exthandler code again and update this patch. I used the chance to do some whitespace changes to the patch (I just hate spaces after single opening brackets...) and I have removed the unnecessary prototypes for helper functions and instead moved those above the location where they are used. Other minor changes can be seen by file-comparing the old to the new patch (setting whitespace changes to be ignored).

Well, it seems to work, at least as well as in my enhanced OS/2 packages. As the icon stuff only crashed Thunderbird (IIRC), I have set sUseRws = PR_FALSE in nsIconChannel.cpp only for TB. Other apps should still be able to benefit from that. Only that I don't see the icons in FF or SM?! We can fix that for real once we have gotten this beast in.

What I don't like is the stuff with the *OS2 strings in unknownContentType.properties. I mean it was really me who got Rich to invent that, but I'm having difficulties to imagine how non-OS/2 translators can do anything meaningful with those class names... The few hardcoded classes I also don't like any more. Is there no other way to query the name of the class? How does the WPS know what name to put in the menu? (I don't have any of the classes installed, so I cannot try.)

Finally, I don't see "WPS Default" anywhere in the UI, do I have to do something special to provoke that? Instead I get "<ProgramName> (default)" (from the defaultApp property) which I actually like more. And shouldn't we be able to get "File" (or better "file") from somewhere else rather than adding a new property, only for OS/2?

Comment 49

[Peter Weilbacher]

(In reply to comment #48)
> What I don't like is the stuff with the *OS2 strings in
> unknownContentType.properties. I mean it was really me who got Rich to invent
> that, but I'm having difficulties to imagine how non-OS/2 translators can do
> anything meaningful with those class names... The few hardcoded classes I also
> don't like any more. Is there no other way to query the name of the class? How
> does the WPS know what name to put in the menu? (I don't have any of the
> classes installed, so I cannot try.)

I have seen the MM classes in action on my wife's desktop now. I think I would replace the "MM" in front of the strings with "MMOS2" or get rid of that completely and only display e.g. Video Player as MM doesn't really tell the user anything. Similarly for "OD" which I guess stands for "Object Desktop"...

Aren't there any other possible classes out there that create menu entries in the WPS Open As menu that we should list here? Perhaps I should test what the new OpenOffice OOWPS stuff does...

> Finally, I don't see "WPS Default" anywhere in the UI, do I have to do
> something special to provoke that?

I studied the code again and it seems "WPS Default" should be displayed when the RWS classes are not registered. I deregistered it but I still couldn't make it appear. Rich, any hints?

> And shouldn't we be
> able to get "File" (or better "file") from somewhere else rather than adding a
> new property, only for OS/2?

There already is a fileType property about which even tells us (via %S) where to add what type it is so that for l10n the right order is preserved.

Comment 50

[Peter Weilbacher]

As Rich already pointed out on the newsgroup a few times, this isn't actually just an enhancement (any more), without this feature the Firefox helper apps dialog is downright broken on OS/2 because it just lets us save files but nothing else...

Comment 51

[Peter Weilbacher]

If we ever want to get this in it will probably have to be in the next days. Andy apparently has tested the patch (from the comment in bug 409733).

Dave, Walter, do you have time to test it, too?

Who knows about Object Desktop? Was it ever released in other languages than English? If not we should really hardcode those two strings (without OD in them). Do these strings well enough represent the menu items that the OD classes create in the Open As menu?

Comment 52

[Peter Weilbacher]

biesi: are you content with Rich's answer from comment 46 or should I try to rewrite the patch to merge Get GetFromTypeAndExtension into GetMIMEInfoFromOS? What exactly would be the advantage of that? As this is OS/2 only I don't think we need sr any more, but if you have easy-to-fix suggestions we should of course try them.

Comment 53

[Peter Weilbacher]

Comment on attachment 295040 [details] [diff] [review]
RWS integration, new full patch

mkaply: as for your remark in comment 36, I now asked in the l10n newsgroup and Axel thinks that unknownContentType.properties would be the best location.

Can you review the patch?

Comment 54

[Mike Kaply [:mkaply]]

What happens if someone doesn't have RWS installed?

Comment 55

[Peter Weilbacher]

It should just work as well or as badly as before. The RWS symbols are dynamically loaded (see nsRwsServiceInit in nsRwsService.cpp for the DosLoadModule/DosQueryProcAddr calls).

Comment 56

[Peter Weilbacher]

Attachment: proposed strings

OK, this is what I propose for the strings. MIDI should be upper cased, MM and OD removed, and Class Viewer changed so that one can place the class name at the correct position for the language in question. One comment for each entry to alert the translators of the special status of these strings...

Comment 57

[Mike Kaply [:mkaply]]

Comment on attachment 295040 [details] [diff] [review]
RWS integration, new full patch

I did a cursory glance, but I am basically going to rubber stamp this based on the testing you guys are doing.

The dependency between widget and the icon protocol handler concerns me a little, but I don't think there is anything we can do there.

Comment 58

[Dave Yeo]

replying to comment #56
Hi Peter, tried to compile with attachment #295040 [details] [diff] [review] applied and the build dies here
In file included from I:/mozilla/widget/src/os2/nsWidgetFactory.cpp:83:
I:/mozilla/widget/src/os2/nsRwsService.h:44:17: rws.h: No such file or directory
make.exe[6]: *** [nsWidgetFactory.o] Error 1
and sure enough I can't find any rws.h in my tree.

Comment 59

[Andy Willis (abwillis)]

I built yesterday and tested the space issue in bug 409733 file handling seemed to be fine.  I just tested opening my local harddrive and it is not showing icons and it is not giving me any options when I right click on a file (XUL tree is selected).

Comment 60

[Peter Weilbacher]

Dave, sorry, forgot to mention that you need RWS present to compile. Get http://hobbes.nmsu.edu/pub/os2/dev/wps/rws080.zip and after unpacking, add the top level directory to CPLUS_INCLUDE_PATH in setmozenv.cmd. Then rws.h should be found.

Andy, oh yes, I wanted to check out the icons. That the menu isn't displayed in SeaMonkey is because the directory.js part of the patch was dropped (won't work in Firefox anyway). If you want that feature, we should investigate it in a follow-up bug.

Comment 61

[Peter Weilbacher]

(In reply to comment #57)
> The dependency between widget and the icon protocol handler concerns me a
> little, but I don't think there is anything we can do there.

Mike, what scenario are you thinking of where you would build libpr0n/the icon handler but not widget?

Comment 62

[Mike Kaply [:mkaply]]

(In reply to comment #61)
> (In reply to comment #57)
> > The dependency between widget and the icon protocol handler concerns me a
> > little, but I don't think there is anything we can do there.
> 
> Mike, what scenario are you thinking of where you would build libpr0n/the icon
> handler but not widget?
> 

I don't know that there is a scenario, but generally in Mozilla code they try to avoid major cross component dependencies.

But honestly since OS/2 icon code code (in particular how to get WPS icons) is so screwed up, I don't think there is any other way to do this.

Comment 63

[Dave Yeo]

In reply to comment #60
Yes after posting the failure I looked at the top of this bug and hit my forehead with palm :). Anyways first try I used rsw070. Helper apps did not work at all. Then I rebuilt with 080 and just did a couple of tests. It seems to work fine, have an extra choice in Seamonkey doesn't know how to handle this dialog which is the WPS default and also offers the helper app I already have. Good work.

Comment 64

[Walter Meinl]

I want also to confirm that its a nice enhancement and builds fine (after installing RWS080) here with FF.
Concerning OD: at least 1.5 and Professional were also available in German language. Unfortunately after upgrading to ECS2rc's I did not yet reinstall OD 2.0, and I don't remember if I had a German language version of it. You may consider to use odDocumentView or simply odView instead of odTextView, cause OD had viewers for a lot of file types/extensions (4 pages in the handbook). However, probably a lot of them are outdated now. 

Comment 65

[Peter Weilbacher]

Thanks for testing. :-)

Walter, it's more than just editing the text for that. We would also have to know the corresponding class name and default view handle for that more generic OD viewer, otherwise we cannot detect it. I think Rich would have added them if that info would be easily available.

Comment 66

[Rich Walsh]

First, let me apologize for not responding sooner.  For several weeks, my PC has been more dead than alive (continuous TRAP 0002's).  Here are some comments in no particular order;  some may duplicate what Peter has already said.

> (mkaply) - What happens if someone doesn't have RWS installed?

There is no runtime dependency.  If RWS isn't present, the code falls back to standard PM calls to get icons, open files, etc.  Also, it's fairly efficient:  RWS is demand-loaded & if it isn't present, no further attempts to use it are made.

> (pweilbacher) - I don't see "WPS Default" anywhere in the UI, do I have to do something special to provoke that?

As you gathered, this is only used by the fall-back code.  To see it, use rws08.cmd to deregister the class, then remove the RWS dlls from the directory containing the app you want to test (and from any directory on the LIBPATH, if applicable).

> (pweilbacher) - I have removed the unnecessary prototypes for helper functions

*Well-formed* C has prototypes for every function (IMHO).

> (dyeo) nsRwsService.h:44:17: rws.h: No such file or directory

Couldn't rws.h be checked in somewhere so there are no build dependencies?  It was released under the MPL, and I'd be happy to relicense it under the current tri-license scheme.

> (dyeo) - have an extra choice in Seamonkey doesn't know how to handle this dialog which is the WPS default and also offers the helper app I already have.

Are you saying that you're seeing, e.g., both "pmview.exe" and "PMView 2000"?  If so, then it's because there's no way to determine that an exe you added manually matches the exe invoked by a WPS Program object.  The simple solution is not to do manually what it will do for you automatically - and will do better.  There's a major benefit to using the WPS default:  it's identical to dropping the file on a fully-configured WPS Program Object.  Any commandline parameters, session settings, and environment variables you've added to the pgm object will be used.  You can't get any of those by invoking the exe directly.

> (wmeinl) - I don't remember if I had a German language version of it.

I'm virtually certain there was a German version of 2.0

> (wmeinl) - You may consider to use odDocumentView or simply odView instead of odTextView, cause OD had viewers for a lot of file types/extensions. However, probably a lot of them are outdated now.

The ID for the document viewer's menuitem was different than the ID used for TextView;  please don't mess with it.  IIRC, the extra viewers were nearly obsolete by the time 2.0 came out - I'd be surprised if many people still use them.  OTOH, I use good-ole TextView (and ZipFolder) constantly (when my machine is working).

> (pweilbacher) - Aren't there any other possible classes out there that create menu entries in the WPS Open As menu that we should list here?

The items I included were based on what I had available and what were likely to have widespread use (remember, Object Desktop was the best-selling OS/2 app ever).  If you think there are other class viewers with significant usage, go for it - or, just wait for the complaints to come in :-)

Comment 67

[Dave Yeo]

I should of been more clearer about the extra choice. I tested with a postscript file and the WPS default was GSView and the box was filled out with x:\pathto\gvpm.exe due to my using gvpm.exe previously. This is the same as Rich's PMView example.

Comment 68

[Peter Weilbacher]

(In reply to comment #66)
> > (pweilbacher) - I have removed the unnecessary prototypes for helper functions
> 
> *Well-formed* C has prototypes for every function (IMHO).

Where did you get that from? Prototypes only make sense if they are (1) used for documentation, (2) placed in header files included from multiple C files, (3) there would otherwise be circular dependencies, (4) one loads functions from shared libraries dynamically. Otherwise it's just an ugly work around of a design error of C compilers which costs extra time to maintain and which can easily be fixed by moving the respective function above its caller (which is what I did).
[But yes, I know there is a school of programming promoting "main at top"...]

> > (dyeo) nsRwsService.h:44:17: rws.h: No such file or directory
> 
> Couldn't rws.h be checked in somewhere so there are no build dependencies?  It
> was released under the MPL, and I'd be happy to relicense it under the current
> tri-license scheme.

I was thinking before that we should perhaps create a new subdirectory for special OS/2 build dependencies somewhere (but where?), mainly for the Fontconfig and FreeType stuff, but then rws.h could go there, too. This would be strangely out of place anywhere in the current tree. I think other-licenses/ is used for stuff like this. Even if the license can be changed to fit, that could be the place to put it.

> The items I included were based on what I had available and what were likely to
> have widespread use (remember, Object Desktop was the best-selling OS/2 app
> ever).  If you think there are other class viewers with significant usage, go
> for it

I have checked the classes of my installation but none of them create Open As menu entries. I still wonder about things like the CWMM classes and WPSWizard that I have never gotten installed but many other people have (aren't they included in eCS now?).

>  - or, just wait for the complaints to come in :-)

Unfortunately, that's what we cannot do. There will be a string freeze very soon after which we cannot add any more entries. :-(

Comment 69

[Peter Weilbacher]

(In reply to comment #60)
> Andy, oh yes, I wanted to check out the icons.

Oh, once looking into the source file and not just the patch I found out quickly why the icons don't display: bug 333253 added |return NS_ERROR_NOT_IMPLEMENTED;| in the OS/2 version of MakeInputStream() for cairo-os2, of course without telling any OS/2 people...

So let's check this stuff in without the icon handler and then get it working again in another bug that I'm about to file.

Comment 70

[Peter Weilbacher]

Attachment: new full patch for checkin

OK, so this is the new full patch updated to match the final strings from attachment 295768 [details] [diff] [review] and without the icon code. The only code changes are the two cases where %S in the strings needs to be replaced with the real content now.
r+ carried over, will check this in soon.

Comment 71

[Peter Weilbacher]

OK, the patch is in, only took 2.5 years to get this done. ;-) So I'll be relieved of my "duty" to provide enhanced builds for Gecko 1.9.

I will file follow-up bugs on README.txt and on getting rws.h into CVS and make them "block" this bug.

Comment 72

[Axel Hecht [:Pike]]

For reference, the localization notes in this patch look good to me, so this should fairly low-risk to just translate without access to OS/2, IMHO.