Register .nvda-addon extension to be handled by installed copies of NVDA, to easily install new addons

[nvaccessAuto]

Reported by ragb on 2012-05-08 15:37
The usecase for this is almost obvious: when we click on a .nvda-addon package the add-on installation dialog apears, as in the GUI. I supose this can only e done for installed copies.
Don't know the dificulty on implement such a feature but I think the it may easy the user interaction, although not crytical.

Don't think it is that prioritary to target fo 2012.2, unless it is trivial to implement (I don't think so, with my few knoledge of Windows and inter-process communication and shell extensions and such).

[nvaccessAuto]

Comment 1 by mdcurran on 2012-05-08 23:21
The only issue I have is that it may become confusing for people as to which NVDA it will be installed in. We'd probably want to avoid it automatically installing when opening one within a web browser otherwise it may stop people from successfully saving for later installing into a portable copy.

Some implementation ideas:

• If add-ons were never going to show any GUI on install, we could simply register nvda_slave.exe as a file handler for .nvda-addon, and nvda_slave could handle the installation. However NVDA would still need to be restarted of course and conveying this to the user would be hard.
  • Possibly my prefered idea: nvda_slave.exe would be registered as the file handler, but all it would do is tell the currently running NVDA to install the add-on, via an nvdaControllerInternal RPC call. Then NVDA itself could do its normal install add-on path, which includes the progress dialog and possibility for the add-on to show a GUI etc. However, for installed copies of NVDA, this would mean that if a portable happened to be running instead of the installed copy, the add-on would install in the portable copy -- which may or may not be what the user wanted.

I'm quite confident on how to handle the rpc stuff, but registering file handlers is not something I've done before. Originally it was just registry entries in h classesRoot in the registry, but now I think its a bit more secure... not sure.

Probably not a 2012.2 thing as it is a bit of coding and we are fast approaching the beta.

Changes:
Milestone changed from None to 2012.3

[nvaccessAuto]

Comment 2 by jteh (in reply to comment 1) on 2012-05-09 00:05
Replying to mdcurran:

The only issue I have is that it may become confusing for people as to which NVDA it will be installed in.

I'd argue that aside from serious testers, most users who have NVDA installed will almost always use the installed copy on their system. Using both an installed copy and a portable copy is not really necessary for most users. Many will generate a portable from their installed, but that's fine. That said, installing into the currently running copy probably still makes sense.

We'd probably want to avoid it automatically installing when opening one within a web browser otherwise it may stop people from successfully saving for later installing into a portable copy.

I disagree; opening within a web browser is perhaps the most important use case once we have an add-ons repository. There's always Save target as, which we can explain for portables in the user guide/web site.

[nvaccessAuto]

Comment 3 by ragb (in reply to comment 2) on 2012-05-09 00:31
Replying to jteh:

Replying to mdcurran:

The only issue I have is that it may become confusing for people as to which NVDA it will be installed in.

I'd argue that aside from serious testers, most users who have NVDA installed will almost always use the installed copy on their system. Using both an installed copy and a portable copy is not really necessary for most users. Many will generate a portable from their installed, but that's fine. That said, installing into the currently running copy probably still makes sense.

I agree that installing on the currently-running copy makes sence. If it is a portable copy we may warn the user before installation to avoid wrong assumptions.

We'd probably want to avoid it automatically installing when opening one within a web browser otherwise it may stop people from successfully saving for later installing into a portable copy.

I disagree; opening within a web browser is perhaps the most important use case once we have an add-ons repository. There's always Save target as, which we can explain for portables in the user guide/web site.

If the behaviour for installing addons on running portable copies is explicit (with warning or explanation in the manual) I think that there is no problem. And it is surely an important usecase.

[nvaccessAuto]

Comment 4 by ragb on 2012-06-30 14:30
I'll take care of this fo 2012.3.

[nvaccessAuto]

Comment 5 by ragb on 2012-07-01 14:28
Hi,

I believe I need a hint or two about this.

To instruct NVDA to install an add-on I must send a message to the running NVDA process to call the installation routines. My idea is to make nvda_slave call the current process, and have something like a NVDAControllerInternal_installAddon function implemented on nvda helper. Is that right?

If yes, how could I call the nvda helper method from nvda_slave? I do0t need code, just the process. Maybe it is obvious but my windows programming skills don't get it :).

regards,

Rui Batista

[nvaccessAuto]

Comment 6 by jteh (in reply to comment 5) on 2012-07-11 02:10
Replying to ragb:

To instruct NVDA to install an add-on I must send a message to the running NVDA process to call the installation routines. My idea is to make nvda_slave call the current process, and have something like a NVDAControllerInternal_installAddon function implemented on nvda helper. Is that right?

Yes.

If yes, how could I call the nvda helper method from nvda_slave?

In short, the function should be exposed by nvdaHelperRemote.dll, so you load that dll and call the function in that. For an example, see the way nvdaControllerInternal_logMessage is handled (start with 2827a34).

[nvaccessAuto]

Comment 7 by jteh on 2012-08-20 08:59
Rui, are you still working on this/likely to get a chance to work on it in the next few weeks? Obviously, there's no pressure; we're just trying to determine plans for 2012.3.

[nvaccessAuto]

Comment 8 by ragb (in reply to comment 7) on 2012-08-21 16:11
Replying to jteh:

Rui, are you still working on this/likely to get a chance to work on it in the next few weeks? Obviously, there's no pressure; we're just trying to determine plans for 2012.3.

Yes, I'll do it. Actually, today I was just revewing what I already did. Maybe this week I'll have something to show you.

[nvaccessAuto]

Comment 9 by ragb on 2012-08-22 11:24
Branch for this at
http://bzr.nvaccess.org/nvda/addons-shell-integration

For now only gui nvdaHelper and nvda_slave stuff are coded. It is already possible to request add-on installation be calling nvda_slave from the command line.

[nvaccessAuto]

Comment 10 by jteh on 2012-08-23 03:05
This looks good! Thanks! Some code review:

In NVDAHelper.py, you import wx at module level. Because NVDAHelper gets imported quite early, wx should be a late import (in the function requiring it), as it is a huge package. We've probably made this mistake elsewhere anyway, but best not to make it again. :)

To make !AddonsDialog a singleton, you've implemented a getInstance() classmethod and raise an exception if someone tries to make a second instance. Because you truly want a singleton here, it's probably easier and more elegant to just return the instance from new if it already exists:

    def __new__(cls, *args, **kwargs):
        if not AddonsDialog._instance:
            AddonsDialog._instance = super(AddonsDialog, cls).__new__(cls, *args, **kwargs)
        return AddonsDialog._instance

+     if closeAfter:
+         self.onClose(None) # hack...

Did you try calling self.Close() here? I guess it mightn't work because the dialog isn't actually showing. In that case, I'd remove the hack comment, since this makes sense.

In nvda_slave.pyw:

+         h=ctypes.windll.kernel32.LoadLibraryExW(os.path.abspath(ur"lib\nvdaHelperRemote.dll"),0,0x8)
+         if not h:
+             import winUser
+             winUser.MessageBox(0,
+             # Translators: the message that is shown when the user tries to install an add-on from windows explorer and NVDA is not running.

The dll will load even if NVDA is not running. You should instead check the return code from remoteLib.nvdaControllerInternal_installAddonPackageFromPath. If it is non-0, some RPC error occurred, which almost certainly means NVDA wasn't running.

[nvaccessAuto]

Comment 11 by jteh on 2012-08-23 04:07
Oh, one more thing. In nvda_slave.pyw:

+             _("""Can not install NVDA add-on from {path}.
+             You must be running NVDA to be able to install add-ons.""").format(path=addonPath),

You don't want to use a triple-quoted string here because the tabs from your second line will actually be included in the message. You could remove the tabs, but that looks pretty ugly. I'd write it like this:

                _("Can not install NVDA add-on from {path}.\n"
                "You must be running NVDA to be able to install add-ons.").format(path=addonPath),

[nvaccessAuto]

Comment 12 by ragb (in reply to comment 10) on 2012-08-23 11:02
Replying to jteh:

This looks good! Thanks! Some code review:

In NVDAHelper.py, you import wx at module level. Because NVDAHelper gets imported quite early, wx should be a late import (in the function requiring it), as it is a huge package. We've probably made this mistake elsewhere anyway, but best not to make it again. :)

Agreed.

To make !AddonsDialog a singleton, you've implemented a getInstance() classmethod and raise an exception if someone tries to make a second instance. Because you truly want a singleton here, it's probably easier and more elegant to just return the instance from new if it already exists:

  def __new__(cls, *args, **kwargs):
      if not AddonsDialog._instance:
          AddonsDialog._instance = super(AddonsDialog, cls).__new__(cls, *args, **kwargs)
      return AddonsDialog._instance

Yes, this is a real singletone, not the kind we have on settings dialogs.

+       if closeAfter:
+           self.onClose(None) # hack...

Did you try calling self.Close() here? I guess it mightn't work because the dialog isn't actually showing. In that case, I'd remove the hack comment, since this makes sense.

There is a big problem with this call. It is only called if the installation succeeds. Either on error or user cancelation this code is not reached. My solution is to put it on a finaly block, even on exception it would be good to dispose of the dialog.

In nvda_slave.pyw:

+           h=ctypes.windll.kernel32.LoadLibraryExW(os.path.abspath(ur"lib\nvdaHelperRemote.dll"),0,0x8)
+           if not h:
+               import winUser
+               winUser.MessageBox(0,
+               # Translators: the message that is shown when the user tries to install an add-on from windows explorer and NVDA is not running.

The dll will load even if NVDA is not running. You should instead check the return code from remoteLib.nvdaControllerInternal_installAddonPackageFromPath. If it is non-0, some RPC error occurred, which almost certainly means NVDA wasn't running.

Ok thanks. I'm not that confident on this Windows rpc stuff.

[nvaccessAuto]

Comment 13 by mdcurran on 2012-09-12 00:37
All of this is looking good to me, and works well on my system.
However, I do get one error in my log the first time I install, as _deleteKeyAndSubkeys cannot find the progID key. I'd think this WindowsError should be caught either within _deleteKeyAndSubkeys or just outside it, and perhaps a debugWarning or nothing at all. But since this will always occur for new installs, falling to the outer log.error is not useful.

The Open With problem is annoying, though I don't believe there is any way this could be fixed. all other programs also just show their app name. Like it or not its called NVDA slave. We could consider renaming the product name to just NVDA though I don't think this is all that important.
If there's no more work a part from the exception catching, which I can do if you like, I'd like to merge this soon.

[nvaccessAuto]

Comment 14 by ragb (in reply to comment 13) on 2012-09-12 09:24
Replying to mdcurran:

All of this is looking good to me, and works well on my system.

However, I do get one error in my log the first time I install, as _deleteKeyAndSubkeys cannot find the progID key. I'd think this WindowsError should be caught either within _deleteKeyAndSubkeys or just outside it, and perhaps a debugWarning or nothing at all. But since this will always occur for new installs, falling to the outer log.error is not useful.

Makes sence, thanks.

The Open With problem is annoying, though I don't believe there is any way this could be fixed. all other programs also just show their app name. Like it or not its called NVDA slave. We could consider renaming the product name to just NVDA though I don't think this is all that important.

I tried to use the !friendlyAppName key in the windows registry but it seems to have no effect at all. See

http://msdn.microsoft.com/en-us/library/windows/desktop/ee872121%28v=vs.85%29.aspx#applications

Maybe I'm doing something wrong though.

If there's no more work a part from the exception catching, which I can do if you like, I'd like to merge this soon.

Except the application name issue (that as you said we can't probably change anyway) I've nothing to add to this. If you don't mind adding the catching code, please do. Sorry, I've been a bit buzy lately :).
This needs heavy user testing. I could only test it on windows 7 64 bits, english edition. I'm a bit worried about hearly windows versions. In theory it is all compatible but who knows in practice what happens.

[nvaccessAuto]

Comment 15 by jteh on 2012-09-14 04:07
Merged with some changes by me in c8d9c69. Thanks for your great work.
Changes:
State: closed

[nvaccessAuto]

Comment 16 by ragb (in reply to comment 15) on 2012-09-14 14:19
Replying to jteh:

Merged with some changes by me in c8d9c69. Thanks for your great work.

Thanks. Didn't know about that COM error status thing, sorry. Looks all good.