Vip mud script #1728

Closed
nvaccessAuto opened this Issue Aug 19, 2011 · 10 comments

1 participant

@nvaccessAuto

Reported by dwillemv on 2011-08-19 08:33
As vip mud (a mud client for the blind) is unkind enough to not support the NVDA API, we created a script to allow for speaking of incoming text for inclusion in NVDA. The script appears stable, but it may require further testing.

@nvaccessAuto

Comment 1 by jteh on 2011-08-21 18:41
Thanks for the contribution!

Here is some code review. Please address these comments and update the module.

First, the header comments need to be changed to conform with the rest of the NVDA code. Something like this:

#appModules/vipmud.py
#A part of NonVisual Desktop Access (NVDA)
#Copyright (C) 2011 Willem Venter and Rynhardt Kruger
#This file is covered by the GNU General Public License.
#See the file COPYING for more details.
import speech
from keyboardHandler import KeyboardInputGesture

Unused imports. Please remove.

  def chooseNVDAObjectOverlayClasses(self, obj, clsList):
      if controlTypes.STATE_READONLY in obj._get_states():

Is there any reason you're using _get_states here instead of the property? If not, please change this to obj.states.

class MudText(edit.Edit):
  def event_valueChange(self):

...

      for i in range(0, len(lines)):
          lines[          if len(lines[i](i].strip()
)) >0:self.appModule.msgs.insert(0, lines[```
There are a few issues here:
 * ```lines[i](i])
).strip()``` will have no effect. strip() returns the stripped string, as opposed to stripping the string on which it is called. Strings are immutable.
 * ```if len(s) > 0``` often makes more sense as just ```if s```.
 * It's more elegant to iterate using a list iterator here.
I'd change this code to the following:
    for line in lines:
        line = line.strip()
        if not line:
            continue
        self.appModule.msgs.insert(0, line)

  if len(self.appModule.msgs) > self.appModule.historyLength:

      self.appModule.msgs =self.appModule.msgs[```

This can be done far more efficiently:

        del self.appModule.msgs[self.appModule.historyLength:](0:self.appModule.historyLength]
>)

Note that you don't even need the if check, as it's not an error to delete a slice which doesn't exist; the slice is just considered empty.

Thanks again.

@nvaccessAuto

Comment 2 by dwillemv (in reply to comment 1) on 2011-08-22 09:29
Replying to jteh:
Thank you for the advice and suggestions. I will make the needed changes and update the ticket.

@nvaccessAuto

Comment 3 by dwillemv (in reply to comment 1) on 2011-08-28 13:15
I have updated the module and changed it as required.

@nvaccessAuto

Comment 4 by jteh on 2011-08-29 06:37
Thanks. Looks good.

Just a few final things that I missed in my last review (sorry!):

#This module makes NVDA read incoming text in VIP mud as well as allowing you to review the last nine messages with control 1 through 9

This comment is probably better done as the module docstring. Put something like this just above or below the imports:

"""
App module for VIP Mud
This module makes NVDA read incoming text, as well as allowing the user to review the last nine messages with control 1 through 9.
"""
  def script_readMessage(self,gesture):
      num=int(gesture.mainKeyName[        if len(self.msgs)>num-1:
          ui.message(self.msgs[num-1](-1])
))
      else:
          ui.message(_("No message yet"))

Because Python has exception handling and it's going to check the length anyway, it's probably more efficient to catch exceptions than check the length, like this:

        try:
            ui.message(self.msgs[num-1])
        except IndexError:
            ui.message(_("No message yet"))

I notice you've commented out this line:

#         line =line.strip()

Was this intentional? If so, the line should just be removed.

@nvaccessAuto

Attachment vipmud.py added by dwillemv on 2011-08-29 12:26
Description:

@nvaccessAuto

Comment 5 by dwillemv on 2011-08-29 12:29
Thanks for the corrections. I updated the file again.

@nvaccessAuto

Comment 6 by Bernd on 2012-06-11 22:27
Well, what prevents including this appmodule into main branch?

@nvaccessAuto

Comment 7 by jteh on 2012-06-12 00:50
Changes:
Milestone changed from None to 2012.3

@nvaccessAuto

Comment 8 by dwillemv (in reply to comment 7) on 2012-06-12 05:12
Replying to jteh:
Thank you.

@nvaccessAuto

Comment 9 by jteh on 2012-06-12 06:32
Committed in ca45945. My sincere apologies for the delay; this seems to have slipped our attention.
Changes:
State: closed

@nvaccessAuto nvaccessAuto added this to the 2012.3 milestone Nov 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment