ability to add more punctuation symbols in the Punctuation/Symbols Pronunciation dialog box

[nvaccessAuto]

Reported by blindbhavya on 2014-08-02 16:54
There are several symbols that NVDA (or rather/maybe ESpeak) does not read.
Currently, the user needs to copy paste the symbol in the Default Default Dictionary > Add > Pattern edit box and then type in the Replacement pronunciation text.
So, I propose that there should be an Add button in the Punctuation/Symbol Pronunciation dialog box called 'Add button' (shortcut Alt + A) by which the user can add more punctuation marks or technical symbols.
I feel the necessity of this feature despite the power of the Speech Dictionaries because in the Punctuation/SYmbol Pronunciation dialog box, one can set the punctuation/symbol level at which the symbol should be spoken, which is otherwise inachievable.
Hope I was clear.
Blocking #4563

[nvaccessAuto]

Comment 2 by norrumar on 2014-10-24 09:18
I feel this interesting. But, if a button is added to add symbols, imo another button should allow to delete or edit them.
The dialog to add new symbols could have these components:

• Pattern.
• Replacement.
• Symbol type (simple or complex).
• Punctuation level.
  Thanks.

[nvaccessAuto]

Comment 4 by jteh on 2014-10-24 09:51
It should be integrated into the existing Symbol Pronunciation dialog. The reason I didn't do this in the first place is that it's tricky to get the code to distinguish between custom symbols and built-in symbols that have been modified. It's not possible to remove built-in symbols, so the Remove button must be disabled in that case.

[nvaccessAuto]

Comment 5 by jteh on 2014-10-24 09:55
Btw, it will never be possible for users to add complex symbols. Symbol pronunciation is compiled into one large regular expression, so an error in a regular expression segment or a misunderstanding of how the symbol pronunciation code works can severely break things. It is simply too dangerous to allow user entry for these.

[nvaccessAuto]

Comment 6 by norrumar on 2014-10-24 10:06
Thanks for the explanation.
Then an add button can be use, similar to the speech dictionaries dialog, and when a symbol is added, a comment like Custom could be added to the corresponding symbol to distinguish the builting and customized, though perhaps is a dirty way.
I can create a branch for this if was useful.
Thanks.

[nvaccessAuto]

Comment 7 by blindbhavya on 2014-10-24 10:11
Hi,
Could you give a quick example of a simple and complex symbol, and their differences? I did a very quick google search, and nothing useful or relevant turned up.
Thanks.

[nvaccessAuto]

Comment 8 by norrumar (in reply to comment 7) on 2014-10-24 10:34
Replying to blindbhavya:

Hi,

Could you give a quick example of a simple and complex symbol, and their differences? I did a very quick google search, and nothing useful or relevant turned up.

Thanks.

Please, see 2.2. Symbol pronunciation, of the NVDA Developer guide at:
http://community.nvda-project.org/documentation/developerGuide.html

[nvaccessAuto]

Comment 9 by jteh on 2014-10-24 10:34
From the NVDA Developer Guide:

Complex symbols are symbols which aren't simply a character or sequence of characters, but instead require a more complicated match. An example is the full stop (.) sentence ending in English. The "." is used for multiple purposes, so a more complicated check is required to determine whether this refers to the end of a sentence.

[nvaccessAuto]

Comment 10 by blindbhavya on 2014-10-24 10:46
Thanks

[nvaccessAuto]

Comment 11 by jteh (in reply to comment 6) on 2014-10-24 10:49
Replying to norrumar:

Then an add button can be use, similar to the speech dictionaries dialog, and when a symbol is added, a comment like Custom could be added to the corresponding symbol to distinguish the builting and customized, though perhaps is a dirty way.

As you say, that's pretty ugly and it also isn't particularly user or localisation friendly.

It just occurred to me that since we only need to know whether to enable/disable the Remove button as we move to each item in the list (i.e. one item at a time), we can just check whether the symbol is present in sources other than user defined symbols at that point. That will require some changes to SymbolProcessor, but it shouldn't be too drastic.

[nvaccessAuto]

Comment 12 by norrumar (in reply to comment 11) on 2014-10-24 16:10
Replying to jteh:

Replying to norrumar:

Then an add button can be use, similar to the speech dictionaries dialog, and when a symbol is added, a comment like Custom could be added to the corresponding symbol to distinguish the builting and customized, though perhaps is a dirty way.

As you say, that's pretty ugly and it also isn't particularly user or localisation friendly.

It just occurred to me that since we only need to know whether to enable/disable the Remove button as we move to each item in the list (i.e. one item at a time), we can just check whether the symbol is present in sources other than user defined symbols at that point. That will require some changes to SymbolProcessor, but it shouldn't be too drastic.

I agree with you.
Thanks.

[nvaccessAuto]

Comment 13 by norrumar on 2014-10-26 22:05
I have started to work on this on t4354 branch at
https://bitbucket.org/nvdaaddonteam/nvda

At this point it's possible to add new symbols, but not remove them, and it's not disallowed to add the same symbol many times.
Thanks.

[nvaccessAuto]

Comment 14 by jteh on 2014-10-26 22:09
Let us know when you need code review for this.

[nvaccessAuto]

Comment 15 by norrumar (in reply to comment 14) on 2014-10-28 18:29
Replying to jteh:

Let us know when you need code review for this.

Hi, the work is incomplete, but if you want can review it or give me some recommendations.

1. In the speech dictionaries, it's possible to add an entry different times. What to do in speech symbols?
2. In my branch, symbols as , (comma), if changed, can be reseted with the remove button, though now it is not reflected in the dialog until the OK button is pressed on the symbols list. But perhaps this is confusing and changed symbols, if haven't been added by the user, shouldn't be deleted.
   Thanks.

[nvaccessAuto]

Comment 16 by jteh (in reply to comment 15) on 2014-10-28 22:03
Replying to norrumar:

1. In the speech dictionaries, it's possible to add an entry different times. What to do in speech symbols?

I'm not sure what you mean by "different times" here. If you mean order, the order doesn't matter. If you mean duplicates, duplicates should not be allowed.

1. In my branch, symbols as , (comma), if changed, can be reseted with the remove button

I agree that this is potentially unintuitive. I think the Remove button should be disabled for these. This is what I meant about the need for this distinction in SymbolProcessor; see comment:11.

[nvaccessAuto]

Comment 17 by norrumar (in reply to comment 16) on 2014-10-28 22:35
Replying to jteh:

Replying to norrumar:

1. In the speech dictionaries, it's possible to add an entry different times. What to do in speech symbols?

I'm not sure what you mean by "different times" here. If you mean order, the order doesn't matter. If you mean duplicates, duplicates should not be allowed.

I mean duplicates.

1. In my branch, symbols as , (comma), if changed, can be reseted with the remove button

I agree that this is potentially unintuitive. I think the Remove button should be disabled for these. This is what I meant about the need for this distinction in SymbolProcessor; see comment:11.

I have started to work on this. See
Changeset: f38c45c8a88e
Branch: t4354
Thanks.

[nvaccessAuto]

Comment 18 by norrumar on 2014-11-01 10:06
Hi, I have finished my work on this ticket for now.
You can see branch t4354 at
https://bitbucket.org/nvdaaddonteam/nvda
Thanks.

[nvaccessAuto]

Comment 20 by jteh on 2014-11-04 03:54
Thanks for your work! Here is some code review.

You use the built-in symbols for the current locale to check whether a symbol is built in. However, for non-English locales, the English symbols are also used as a base, so you actually need to check two sets of symbols. I'd suggest that you keep a list of built-in sources on SpeechSymbolProcessor and add a SpeechSymbolProcessor.isBuiltin method which takes a symbol identifier and returns True if it's present in any of the built-in sources.

When adding a new symbol, the SpeechSymbolProcessor.updateSymbol method:

1. shouldn't copy pattern, as users can't insert complex symbols and only complex symbols have patterns.
2. should return True, since a change was made.

SpeechSymbolProcessor.deleteSymbol is currently broken. It needs to delete the item, not call pop on the item itself. I see you don't currently use this, but I think it'd be better if you did.

Regarding SymbolEntryDialog:

• This dialog is only for adding a symbol, not for editing, since editing is done from the main interface. It should probably be called AddSymbolDialog. This also avoids the need for the "Edit Symbol" title which is then overridden later.
• I'm wondering whether it should just allow the user to enter a symbol and then hit OK, rather than presenting the other fields. This would assign a default level (probably all) and the replacement would be empty. This avoids the need for a separate interface to make changes to the symbol; it's just as if they're editing any other symbol.
• The label "Pattern" is misleading for the first field, especially since complex symbols can have a pattern which is a regular expression. I'd probably just call it "Symbol".
• If replacement is ever empty, the symbol shouldn't be saved. This can probably just be checked in onOk when updating the symbols.

You use wx.NewId and then bind events based on id. Instead, it's clearer to forget ids and instead just bind to the button directly:

        addButton=wx.Button(self,label=_("&Add"),wx.DefaultPosition)
        addButton.Bind(wx.EVT_BUTTON,self.OnAddClick)

+     if symbol.identifier not in self.symbolProcessor.builtinSymbols.symbols.keys(): # and symbol.identifier in self.symbolProcessor.userSymbols.symbols.keys() or symbol in self.addedSymbols:
+         self.removeButton.Enable()
+     else:
+         self.removeButton.Disable()

1. This is where you'd use the isBuiltin method I described above.
2. For future reference, when testing if something is in a dict, just use "item in dict" rather than "item in dict.keys()". The former just does a hash lookup, whereas the latter has to build a list of all of the keys and is therefore quite wasteful.
3. There's a boolean Enabled property on wx controls, so you can just do:
   self.removeButton.Enabled = self.symbolProcessor.isBuiltin(symbol.identifier)
4. There's some commented out code here and also a few other places. Please remove it for the final patch. :)

You seem to delete all items in symbolsList and then rebuild it whenever an entry is added or removed. This isn't efficient and I don't think you need to do this. Just add or remove the relevant item from the list. I also don't follow the need for the code that loops through and clears selection.

The Remove button actually removes the symbol from SpeechSymbolProcessor. This means that if the user presses Cancel, it won't revert the changes. Instead, I think you'll need to keep a list of symbols for pending removal and only remove in onOk. This means you'll also have to remove it from that list if the same symbol is added again.

Thanks!

[nvaccessAuto]

Comment 21 by norrumar (in reply to comment 20) on 2014-11-06 06:41

Replying to jteh:

Thanks for your work! Here is some code review.

Thank you for the revision!

You use the built-in symbols for the current locale to check whether a symbol is built in. However, for non-English locales, the English symbols are also used as a base, so you actually need to check two sets of symbols. I'd suggest that you keep a list of built-in sources on SpeechSymbolProcessor and add a SpeechSymbolProcessor.isBuiltin method which takes a symbol identifier and returns True if it's present in any of the built-in sources.

Hope is done now.

When adding a new symbol, the SpeechSymbolProcessor.updateSymbol method:

1. shouldn't copy pattern, as users can't insert complex symbols and only complex symbols have patterns.
2. should return True, since a change was made.

I think it's done.

SpeechSymbolProcessor.deleteSymbol is currently broken. It needs to delete the item, not call pop on the item itself. I see you don't currently use this, but I think it'd be

better if you did.

I'm using it to add symbols to pendingRemovals.

Regarding SymbolEntryDialog:

• This dialog is only for adding a symbol, not for editing, since editing is done from the main interface. It should probably be called AddSymbolDialog. This also avoids the need for the "Edit Symbol" title which is then overridden later.
• I'm wondering whether it should just allow the user to enter a symbol and then hit OK, rather than presenting the other fields. This would assign a default level (probably all) and the replacement would be empty. This avoids the need for a separate interface to make changes to the symbol; it's just as if they're editing any other symbol.
• The label "Pattern" is misleading for the first field, especially since complex symbols can have a pattern which is a regular expression. I'd probably just call it "Symbol".
• If replacement is ever empty, the symbol shouldn't be saved. This can probably just be checked in onOk when updating the symbols.

I think it's done.

You use wx.NewId and then bind events based on id. Instead, it's clearer to forget ids and instead just bind to the button directly:

      addButton=wx.Button(self,label=_("&Add"),wx.DefaultPosition)
      addButton.Bind(wx.EVT_BUTTON,self.OnAddClick)

I don't use defaultPosition. The sintax you are using seems to produce an error, and it seems to be fixed adding poss=wx.DefaultPosition, but perhaps we don't need this.

+       if symbol.identifier not in self.symbolProcessor.builtinSymbols.symbols.keys(): # and symbol.identifier in self.symbolProcessor.userSymbols.symbols.keys() or symbol in self.addedSymbols:
+           self.removeButton.Enable()
+       else:
+           self.removeButton.Disable()

1. This is where you'd use the isBuiltin method I described above.

2. For future reference, when testing if something is in a dict, just use "item in dict" rather than "item in dict.keys()". The former just does a hash lookup, whereas the latter has to build a list of all of the keys and is therefore quite wasteful.

3. There's a boolean Enabled property on wx controls, so you can just do:

   self.removeButton.Enabled = self.symbolProcessor.isBuiltin(symbol.identifier)
   

I have implemented your suggestions, with some changes, addressing pendingRemovals.

1. There's some commented out code here and also a few other places. Please remove it for the final patch. :)

Hope it's done.

You seem to delete all items in symbolsList and then rebuild it whenever an entry is added or removed. This isn't efficient and I don't think you need to do this. Just add or remove the relevant item from the list. I also don't follow the need for the code that loops through and clears selection.

Yes, I don't know why when I started to work with this got errors and I fixed them using this inefficient code.

The Remove button actually removes the symbol from SpeechSymbolProcessor. This means that if the user presses Cancel, it won't revert the changes. Instead, I think you'll need to keep a list of symbols for pending removal and only remove in onOk. This means you'll also have to remove it from that list if the same symbol is added again.

Hope it's fixed with pendingRemovals and restore button.

Thanks!

Thank you.

[nvaccessAuto]

Comment 22 by jteh (in reply to comment 21) on 2014-11-06 06:52
Thanks. I haven't reviewed the new code yet, but thought I'd reply to some points:

Replying to norrumar:

I don't use defaultPosition. The sintax you are using seems to produce an error, and it seems to be fixed adding poss=wx.DefaultPosition, but perhaps we don't need this.

Sorry. Bad copy and paste error on my part. :( Yes, you can remove the pos argument altogether, since DefaultPosition is the default.

Yes, I don't know why when I started to work with this got errors and I fixed them using this inefficient code.

Did you find a better fix for these errors? (If you've already fixed it in the new code, great!) If not, do you remember what the errors were?

[nvaccessAuto]

Comment 23 by norrumar (in reply to comment 22) on 2014-11-06 10:28
Replying to jteh:

Thanks. I haven't reviewed the new code yet, but thought I'd reply to some points:

Replying to norrumar:

I don't use defaultPosition. The sintax you are using seems to produce an error, and it seems to be fixed adding poss=wx.DefaultPosition, but perhaps we don't need this.

Sorry. Bad copy and paste error on my part. :( Yes, you can remove the pos argument altogether, since DefaultPosition is the default.

OK.

Yes, I don't know why when I started to work with this got errors and I fixed them using this inefficient code.

Did you find a better fix for these errors? (If you've already fixed it in the new code, great!) If not, do you remember what the errors were?

I don't get none error now.
I have tested the symbols dialog adding · (midle dot), and removing it before pressing OK, and once added this symbol, opening the dialog again and removing it.
Thanks.

[nvaccessAuto]

Comment 24 by jteh on 2014-11-07 03:46
More code review. :)

In SpeechSymbolProcessor:

+     self.builtinSources = [```
1. This isn't going to have the effect you want. That creates a list and puts the object inside that list, so the list will have only one item.
2. Even if it did, it copies a large amount of data and has ot be searched from top to bottom.
3. Instead, create a list of the actual source objects, then loop through them in isBuiltin, testing each one. Actually, you can do this in one shot with the any() function; something like: ```return any(identifier in source.symbols for source in self.builtinSources)```

      sources.append(self.localeSymbols.fetchLocaleData("en")[0](builtin.symbols]

))

•     self.builtinSources.extend(self.localeSymbols.fetchLocaleData("en")[0].symbols)
  

Please use a variable, rather than fetching twice.

I like the new approach you've taken in updateSymbol. Nice.

Regarding symbol deletion:

1. The pending removals list should be part of the UI, not the SpeechSymbolProcessor. updateSymbol takes effect immediately, and therefore, so should deleteSymbol.
2. I think having the Restore button is unnecessary/confusing; the symbol remains in the list, so it's not obvious to the user that it is going to be removed unless they see the Restore button. Just remove it from the UI list and the UI copy of the symbol data as soon as the button is pressed.
3. When the Remove button is pressed, the UI should store it in pendingRemovals. onOk should then call deleteSymbol on anything in pendingRemovals. If the user adds an identifier already in pendingRemovals, just remove it from pendingRemovals.
   Does this make sense?

Thanks!

[nvaccessAuto]

Comment 25 by norrumar (in reply to comment 24) on 2014-11-08 06:07
Replying to jteh:

More code review. :)

In SpeechSymbolProcessor:

+       self.builtinSources = [> ```
1. This isn't going to have the effect you want. That creates a list and puts the object inside that list, so the list will have only one item.

Oh, sorry. Sure.

1. Even if it did, it copies a large amount of data and has ot be searched from top to bottom.
2. Instead, create a list of the actual source objects, then loop through them in isBuiltin, testing each one. Actually, you can do this in one shot with the any() function; something like: return any(identifier in source.symbols for source in self.builtinSources)

Done, with symbolIdentifier instead of identifier.

            sources.append(self.localeSymbols.fetchLocaleData("en")[0](builtin.symbols]

))

•       self.builtinSources.extend(self.localeSymbols.fetchLocaleData("en")[0].symbols)
  

Please use a variable, rather than fetching twice.

Thanks, I had done it with other reviewer, but I didn't know your opinion. Done.

I like the new approach you've taken in updateSymbol. Nice.

Thanks.

Regarding symbol deletion:

1. The pending removals list should be part of the UI, not the SpeechSymbolProcessor. updateSymbol takes effect immediately, and therefore, so should deleteSymbol.

I think its right now.

1. I think having the Restore button is unnecessary/confusing; the symbol remains in the list, so it's not obvious to the user that it is going to be removed unless they see the Restore button. Just remove it from the UI list and the UI copy of the symbol data as soon as the button is pressed.

Done. I don't know too much about what users can prefer.

1. When the Remove button is pressed, the UI should store it in pendingRemovals. onOk should then call deleteSymbol on anything in pendingRemovals. If the user adds an identifier already in pendingRemovals, just remove it from pendingRemovals.

Does this make sense?

Yes, I think it's done.

Thanks!

Thank you for your suggestion and revision.

[nvaccessAuto]

Comment 26 by norrumar on 2014-11-08 08:07
Please, wait before reviewing this.
I need to check any() function in isPendingRemoval().
I didn't know that function before and I think it can be simplified.
Thanks.

[nvaccessAuto]

Comment 27 by norrumar on 2014-11-08 18:09
Hi, you can review this when you want. I'm not going to change it for now, at least you give me new feedback.
I have checked any() function, and I think it's right.
Thanks.

[nvaccessAuto]

Comment 28 by norrumar on 2014-11-10 12:34
I thought this was all done, but I have changed my opinion after checking that isPendingRemoval was not having the effect I wanted.
I have removed it and implemented an error dialog to prevent duñlicates can be added.
Thanks.

[nvaccessAuto]

Comment 29 by jteh on 2014-11-11 06:26
Thanks. Most of this is pretty good. However, I've gone ahead and finalised it myself, just because it's a bit faster for me to do it rather than going through a few more iterations. Here's what I changed:

• Added code documentation for deleteSymbol and isBuiltin.
• Fixed/added some translator comments.
• In AddSymbolDialog, use a horizontal sizer so the label appears to the left of the edit field instead of above it. Also, centre the dialog on screen.
• Made pendingRemovals a dict rather than a list for more efficient removal.
• Toggle the enabled state of Remove button in onListItemFocused instead of onSymbolEdited so it gets updated as soon as the item gets focused in the list. Previously, the user had to tab through to make it change.
• Removed wx.CallAfter around gui.messageBox, as it isn't necessary.
• Removed a level of indentation in onAddClick by returning early if the dialog is cancelled.
• Fixed a case where the Add dialog wasn't cleaned up.
• Corrected duplicate warning. The symbol might be built in, not customised, so it shouldn't say it's already in customised symbols.
• When appending an item to a wx.ListCtrl, it returns the index, so just use that rather than wx.ListCtrl.GetItemCount().
• Focus the next item after removing a symbol.
• Fixed minor nits.
• Added documentation in User Guide.

Thanks for all your work.

[nvaccessAuto]

Comment 30 by James Teh <jamie@... on 2014-11-11 06:27
In [852833c]:

Merge branch 't4354' into next

Incubates #4354.

Changes:
Added labels: incubating

[nvaccessAuto]

Comment 31 by norrumar on 2014-11-11 06:28
Hi.
I have disallowed to add empty symbols, since imo it can be confusing.
I have fixed an index error when removing non last symbols too.
You can review all this.
Thanks a lot.

[nvaccessAuto]

Comment 32 by James Teh <jamie@... on 2014-11-11 07:02
In [3a274ca]:

Merge branch 't4354' into next

Incubates #4354.

[nvaccessAuto]

Comment 33 by jteh (in reply to comment 31) on 2014-11-11 07:07
Replying to norrumar:

I have disallowed to add empty symbols, since imo it can be confusing.

It's not only confusing; it causes brokenness. :)

I have fixed an index error when removing non last symbols too.

I fixed a further issue related to this. editingItem needs to be set as well. Otherwise, you get exceptions and other brokenness when you tab out of replacement/level.

Nice catches!

Btw, I squashed all of your original commits into one for the official t4354 branch. It'd be great if you can make any fixes against that. To do this, switch to your t4354 branch and:

git fetch nvaccess t4354
git reset --hard nvaccess/t4354
git push -f

[nvaccessAuto]

Comment 34 by norrumar (in reply to comment 33) on 2014-11-11 08:13
Replying to jteh:

Replying to norrumar:

I have disallowed to add empty symbols, since imo it can be confusing.

It's not only confusing; it causes brokenness. :)

Thanks. I thought so, but I wasn't sure.

I have fixed an index error when removing non last symbols too.

I fixed a further issue related to this. editingItem needs to be set as well. Otherwise, you get exceptions and other brokenness when you tab out of replacement/level.

Yes, thanks.

Nice catches!

Btw, I squashed all of your original commits into one for the official t4354 branch. It'd be great if you can make any fixes against that. To do this, switch to your t4354 branch and:

git fetch nvaccess t4354
git reset --hard nvaccess/t4354
git push -f

Done, thanks.
I have learnt a lot with your revision, and I knew that this was slower for you than make it yourself. :)
Thanks again.

[nvaccessAuto]

Comment 35 by jteh (in reply to comment 34) on 2014-11-11 09:52
Replying to norrumar:

I have learnt a lot with your revision, and I knew that this was slower for you than make it yourself. :)

Thanks again.

You're welcome. Ideally, I would have gone through further code review iterations, as it helps developers to learn and thus we get even better code in future. :) However, I've got a lot on my plate right now and I wanted to make sure this didn't get neglected.

[nvaccessAuto]

Comment 36 by James Teh <jamie@... on 2014-12-03 04:44
In [580eaf5]:

You can now add new symbols in the Symbol Pronunciation dialog.

Authors:    Noelia Ruiz Martínez nrm1977@gmail.com, James Teh jamie@nvaccess.org
Fixes #4354.

Changes:
Removed labels: incubating
State: closed

[nvaccessAuto]

Comment 37 by jteh on 2014-12-03 04:47
Changes:
Milestone changed from None to 2015.1